SHParserST80>resolvePartial: and Sets with nil elements

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

SHParserST80>resolvePartial: and Sets with nil elements

timrowledge
Whilst using Monticello’s ‘browse’ option to look at some code I tripped over a problem I just knew was going to be annoying someday - Sets with actual nils included as elements. That means that any enumeration has to explicitly handle the possibility of an item being a nil, instead of the normal and proper expectation that nils mean nothing and don’t get involved in Set>do:.

So as a result in SHParserST80>resolvePartial: we have
                                c sharedPools do: [:p | (p hasBindingThatBeginsWith: aString) ifTrue: [^#incompleteIdentifier]].
where c is an SHMCClassDefinition and the result of the #sharedPools should be a properly empty set.

However, SHMCClassDefinition>sharedPools
        | d |
        d := Set new.
        classDefinition poolDictionaries do:[:each |
                d add: (Smalltalk at: each asSymbol ifAbsent:[nil]) ].
        ^d

… carefully adds a nil to the Set. So we get ‘nil hasBinding…’ and kaboom.

*If* I assume that the real expectation is for the shared pools list to be like those elsewhere and not include nils, then a simple fix to
        | d |
        d := Set new.
        classDefinition poolDictionaries do:[:each |
                Smalltalk at: each asSymbol ifPresent:[:v| d add: v] ].
        ^d
… appears to make things happier. However, I don’t know anything about the intended working of Shout and I’d rather not simply stomp around in here. Also the code as-was has been around since 2010 so maybe something else has more recently changed and this is not the right thing to fix.

Committed to inbox in the hope someone au fait with the code can look into it.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Don't document the program; program the document.



Reply | Threaded
Open this post in threaded view
|

Re: SHParserST80>resolvePartial: and Sets with nil elements

David T. Lewis
On Wed, Aug 16, 2017 at 03:31:40PM -0700, tim Rowledge wrote:

> Whilst using Monticello???s ???browse??? option to look at some code I tripped over a problem I just knew was going to be annoying someday - Sets with actual nils included as elements. That means that any enumeration has to explicitly handle the possibility of an item being a nil, instead of the normal and proper expectation that nils mean nothing and don???t get involved in Set>do:.
>
> So as a result in SHParserST80>resolvePartial: we have
> c sharedPools do: [:p | (p hasBindingThatBeginsWith: aString) ifTrue: [^#incompleteIdentifier]].
> where c is an SHMCClassDefinition and the result of the #sharedPools should be a properly empty set.
>
> However, SHMCClassDefinition>sharedPools
> | d |
> d := Set new.
> classDefinition poolDictionaries do:[:each |
> d add: (Smalltalk at: each asSymbol ifAbsent:[nil]) ].
> ^d
>
> ??? carefully adds a nil to the Set. So we get ???nil hasBinding?????? and kaboom.
>
> *If* I assume that the real expectation is for the shared pools list to be like those elsewhere and not include nils, then a simple fix to
> | d |
> d := Set new.
> classDefinition poolDictionaries do:[:each |
> Smalltalk at: each asSymbol ifPresent:[:v| d add: v] ].
> ^d
> ??? appears to make things happier. However, I don???t know anything about the intended working of Shout and I???d rather not simply stomp around in here. Also the code as-was has been around since 2010 so maybe something else has more recently changed and this is not the right thing to fix.
>
> Committed to inbox in the hope someone au fait with the code can look into it.

I have no particular expertise here, but your fix looks good. The original code
seem to be abusing its knowledge that the implementation of Set would silently
ignore an object added to the set if the object happens to be an instance of
UndefinedObject. So changing the method so that it no longer assumes that
adding nil is the same thing as not adding anything at all seems right.

Dave