21475 Use ifNil: in HashedCollection grow

[astares]

https://pharo.fogbugz.com/f/cases/21475/Use-isNil-in-HashedCollection-grow

[astares]

Test failures unrelated

[MarcusDenker]

for #isNil, if it is done in a very very tight loop, it might be a bit slower than == nil. But the good thing is that we do have #ifNil:IfNotNil: which compiles to the same byte code at == nil ifTrue:
(just in case you do some extremely performance critical change)

Another thing we did run into in the past is message send vs. none: #isNil does an interrupt check, == nil not. In class Process this can lead to problems (in other real world use cases normaly not).

[dionisiydk]

I think "== nil" here is to ensure that we do not send any message to contained objects. It is important when we work with some "naked objects" which has no messages at all.
And many tools like inspectors do not know what kind of objects they are working with. But they use core collections.

Maybe core collections are already not safe for such cases. But if they are, it will break them.

[dionisiydk]

And from this point of view "== nil" is better then ifNotNil because it is explicitly have no message send. While ifNotNil optimisation can be disabled in system

[astares]

That optimisation can be disabled in system is a technical detail.

For readability I would also vote for #ifNotNil: as also Herby Vojčík wrote.

[dionisiydk]

It is not technical detail.
If Collection is supposed to ensure "no message send" invariant then usage of #ifNotNil: breaks it. Because it is message send from the language point of view.

[astares]

@dionisiydk
Maybe I miss something obvious about the point you would like to discuss here.

For me #== and #ifNotNil: are both in the Kernel, both are understand from ProtoObject(s) downwards ... so whatever naked object the collection contains it should be able to work with/understand both. No?

[dionisiydk]

You can always create class from nil without ProtoObject subclassing. Really safe container should assume nothing about underlying objects.

But now I realized that HashedCollections are not safe containers anyway. They send hash related messages to objects. So it is fine to use ifNotNil: here. Sorry for the noise.