Hi Martin,
On Apr 15, 2018, at 11:40 AM, Martin McClure <martin@hand2mouse.com> wrote:
On 04/15/2018 07:42 AM, Ben Coman wrote: The greater prominence of Critiques in Calypso encourages me to try to clean them out.
I bumped into a false positive "Temporaries read before written." that I've condensed to the following example.
test |x| [ x := nil ] whileNil: [ x ifNil: [ x := 1] ]
Now before I log an Issue, is it feasible to be able to recognise this? Perhaps only for common looping idioms?
In this example, the first runtime reference to x is to send it #ifNil:. So technically, x *is* being read before being written, at least if you count sending it a message as "reading" (which seems a reasonable interpretation to me).
How so? The first run-time reference to xcould be in either of the two non-unlined blocks. The assignment to x in [ x := nil ] could precede the read if (as of course it does, but we can't guarantee) BlockClosure>>whileNil: chooses to evaluate it before [ x ifNil: [ x := 1] ]. The issue is that the code is ambiguous; it depends on the implementation of whileNil: and hence should be reported as "may" rather than "shall".
Anyway, the workaround is simple enough... test |x| x := nil. "silence critiques" [ x := nil ] whileNil: [ x ifNil: [ x := 1] ]
Probably not a terrible idea to be explicit about initializing to nil, thereby revealing the developer's intent that this variable be nil rather than relying on the default initialization to nil.
I've never agreed with this. It is in the language spec that all pointer variables, including temporaries, are initialized with nil, and repeating an initialization shows ignorance, and makes me suspect the rest of the code. Smalltalk intentionally ensures that all variables are initialised and for good reason. It's similar, but worse, than people adding an explicit ifFalse: [nil] to an ifTrue: when the default return value for a false ifTrue: is indeed nil. We should strive for literacy, not bastardise things for the ignorant since repeating a mistake is a route to establishing the mistake as common parlance hence losing elegance and concision.
So I'd lean towards leaving the critique as-is. If a developer knows what they did was safe, they can either ignore the critique, exclude the critique, or put in the explicit initialization to nil. I think I prefer explicit initialization.
The critique is wrong. It can only say "may be read before written" and a human being in the same situation would soon observe that the receiver block is always evaluated before the argument block and so not earn at all. Benis right; this is a false positive. However, saying "may" is at least in the spirit of the language given the possibility of reimplementing whileNil:.
Regards, -Martin