Hi Tim, Levente,
I considered shadowing a feature until I noticed it’s being frowned upon.
But, since I like the feature, a better option would be to adjust
the highlighting of such variables to make shadowing more obvious.
+1
I may have missed some discussion about this; isn’t it a similar situation as when you intentionally use an undefined variable and get a warning when accepting the method? You get a warning a that’s it.
In case of Process tests it’s an oversight on my part, apologies; I’m enclosing fixed versions of the tests:
ProcessTest>>testResumeTerminatingProcess (semaphore is shadowed) ProcessTest>>testTerminateTerminatingProcess (semaphore is shadowed)
And yes, cases like this create an unnecessary confusion whether shadowing is intentional or an oversight, so at least a comment stating the reason for using a shadowed variable would be helpful, I guess.
Best, Jaromir
From: Levente Uzonyimailto:leves@caesar.elte.hu Sent: Sunday, January 1, 2023 1:20 To: The general-purpose Squeak developers listmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] A lot of shadowed variable names in Squeak 6.0 needing clearing up
Hi Tim,
I have got no problem with shadowed variables. It's a feature to be able to shadow those variables. If you want to get rid of them, then you should consider making the compiler raise an error instead of spamming the transcript with warning-looking messages. If you don't do so, they will creep back despite the current wave of cleanup. But, since I like the feature, a better option would be to adjust the highlighting of such variables to make shadowing more obvious.
On Sat, 31 Dec 2022, tim Rowledge wrote:
An excellent example is Random>>#nextBytes:into:startingAt:
It refers to a temp named 'index' which shadows an instvar. Should the method be just using that instvar, or a different named temp? I *think* the latter but it's possible Levente would suggest otherwise. All I can be sure of is that the RandomTest class reports all ok.
In Random, shadowing is pretty much intentional.
The index instance variable is only accessed by private methods implementing the PRNG logic itself. If you think the name is too generic, then you may want to rename it to nextStateIndex, as it holds to the index of the next state to generate the next random values.
From the Random object's point of view, the name index is okay IMO.
In #nextBytes:into:startingAt:, index is obviously the running index of the ByteArray to be filled up with random values. So, it clearly should not touch the instance variable. If you don't like the name index, I suggest using currentIndex instead.
For the other methods of Random, I do not fancy calling variables "i". I understand why it may be tempting to use single letters or abbreviations, especially without code completion, but I prefer descriptive names. Also, RIP my beautiful methods :(. :)
Levente