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 Uzonyi<mailto:leves at caesar.elte.hu>
Sent: Sunday, January 1, 2023 1:20
To: The general-purpose Squeak developers list<mailto:squeak-dev at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20230101/866a17fe/attachment-0001.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ProcessTest.jar.1.cs
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20230101/866a17fe/attachment-0001.ksh>