On Thursday, 10 January 2013 at 07:32:27 UTC, Jonathan M Davis wrote:
> Same thing. It's just a question of whether the local variable is a parameter
> or not. It's a local variable either way. And my previous point about the only
> time that we make shadowing an error being when you can't actually access the
> variable being shadowed still stands.
>

If is this is the only usefulness of this feature, it's clearly useless. This problem is made up, so solving it is worthless (how often do this problem occurs in language whee it is allowed ? I can guarantee that this occurred to me way less than hitting the dmd error on shadowing).

> I can understand being frustrated by ending up using the wrong variable due to
> shadowing, but there are legitimate cases where making such shadowing a
> warning or error would be _very_ annoying, and there's no technical reason why
> the shadowing is a problem. At worst, it's error-prone. But aside from POD
> types where the variables are public, the normal thing to do is to name
> private member variables slightly differently from other variables (e.g.
> prepending with _), which completely eliminates the problem. I would _much_
> rather put up with the occasional problem with the shadowing than have the
> compiler complain about my variable names.
>

I understand what you say here, but is seems to me that this is backward rationalization.

I can assure you that is problem is way more common than the one you mention above, and so, if a language limitation is required for the other one, it is required for that one. Or none is required.

Consider also that this is yet another inconsistency, which is a bad thing in itself.

On Thursday, January 10, 2013 09:07:49 deadalnix wrote:
> Consider also that this is yet another inconsistency, which is a bad thing in itself.

What inconsistency? There's no inconsistency here. Is it that you think that the fact that the local variable shadows the member variable is an inconsistency? The _only_ case where variable shadowing is illegal is when one local variable shadows another local variable, and that's because you can't access the shadowed variable when that happens. In all other cases, you can access the shadowed variable (be it through the this pointer/reference or through . or by using the full import path to the variable). So, if anything is inconsistent, it's the fact that shadowing one local variable with another is illegal. It's legal with everything else and quite easy to work around when it happens.

Or are you arguing that something else here is an inconsistent? Because I don't see it, and I don't know what else from this thread that you could possibly be referring to.

- Jonathan M Davis

On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut wrote:
> After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.
>
>
> class foo
> {
>   float a;
>
>   void doSomething()
>   {
>     float a;
>     a = a * 2.0;
>   }
> }
>
> The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?
>
> Kind Regards
> Benjamin Thaut

There is a rationale for such checks, but I argue that the place of these checks is not in the compiler itself, but in a separate tool.
I don't see a reason why we mix up the correct code (which compiles) with the non error-prone code (which doesn't do things that look buggy). These are two different concepts. D as a systems language and (more importantly) as a powerful generic-programming-enabled language will need to make compromises on both sides. A static code analyzer won't have to. I thing an analyzer for such a flexible language would need to have different levels of ... pickiness.
I'm giving you java and PMD for example - the static code analyzer can be made substantially intelligent in such common places.
So, if we had a solid static code analyzer for D, we wouldn't be having this discussion at all - just add it as a feature of it.
Which brings me to the question - is there any attempt to build such a beast for the D programming language? I think such a community driven tool will be of much value for the language itself (I learned a lot each time about bad design decisions when using the PMD with existing java).
I would argue (but this is a very weak argument) that a decent code editor will be able to visually cue you in the easiest of situations.

> There is a rationale for such checks, but I argue that the place of these checks is not in the compiler itself, but in a separate tool.

This. Compiler should never issue errors on code that may be correct (at least in scope of language coding guidelines). And warnings should just vanish from existence. Checking code for _possible_ bug-prone idioms is job of static analysis tool, not compiler.

On Thursday, 10 January 2013 at 08:35:14 UTC, Jonathan M Davis wrote:
> On Thursday, January 10, 2013 09:07:49 deadalnix wrote:
>> Consider also that this is yet another inconsistency, which is a
>> bad thing in itself.
>
> What inconsistency? There's no inconsistency here. Is it that you think that
> the fact that the local variable shadows the member variable is an
> inconsistency? The _only_ case where variable shadowing is illegal is when one
> local variable shadows another local variable, and that's because you can't
> access the shadowed variable when that happens. In all other cases, you can
> access the shadowed variable (be it through the this pointer/reference or
> through . or by using the full import path to the variable). So, if anything
> is inconsistent, it's the fact that shadowing one local variable with another
> is illegal. It's legal with everything else and quite easy to work around when
> it happens.
>

How many time did you find yourself in a position of where you say to yourself "how no, I can't access that variable anymore because it is shadowed !".

And, if this is really the reason, why is this forbidden ?

void foo() {
    {
        int bar;
    }
    {
        float bar;
    }
}

> Or are you arguing that something else here is an inconsistent? Because I
> don't see it, and I don't know what else from this thread that you could
> possibly be referring to.
>

It is sometime allowed to shadow declaration and sometime not. This is not very consistent. Inconsistency come always at a cost, so the benefit must be very real to create one.

Jacob Carlborg:

> If you need a special naming convention for your member variables you have made something wrong when designing the language.

I agree.
And indeed that enhancement request aims to fix that little part of the language.

Bye,
bearophile

Jonathan M Davis:

> You don't need it. It just makes the code clearer if you do.

It's not just a matter of clarity for the reader. It's a convention that avoids bugs when I write code. In my bug diary I count six or more bugs caused by that in my D code. Plus one more related bug even when I have started to use a convention.


> The language doesn't force anything on

Issue 4407 is indeed proposing a little language change.

Bye,
bearophile

On Wednesday, 9 January 2013 at 21:17:01 UTC, comco wrote:
> On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut wrote:
>> After some refactoring I had a series of bugs that came from renaming class members so that the following situation was present.
>>
>>
>> class foo
>> {
>>  float a;
>>
>>  void doSomething()
>>  {
>>    float a;
>>    a = a * 2.0;
>>  }
>> }
>>
>> The local variable a shadows the member a after the refactoring and therefore this code will no longer work as expected. This was quite time consuming to track down. So I wanted to ask if we want to prevent that with a warning or even an error? D does not allow shadowing of local variables, so why is it allowed for members?
>>
>> Kind Regards
>> Benjamin Thaut
>
> I don't think an error will be appropriate, because this a can be a protected member of the super-super-super class and it won't be nice to be forced to use a different name. But another view on this is that it behaves consistently with the case in which a function parameter shadows a member.
> I won't like this code:
> class Pu {
>    int a, b;
>    this(int a, int b) {
>        this.a = a;
>        this.b = b;
>    }
> }
>
> to issue warnings for a and b.

If you use this, then naturally a warning would be ridiculous.

However this should produce a warning:
a = a;
b = b;

Am 10.01.2013 09:37, schrieb comco:
> There is a rationale for such checks, but I argue that the place of
> these checks is not in the compiler itself, but in a separate tool.

But there are no such tools yet for D.
Couldn't we add a -analyze flag to the compiler which will do such checks? Other compilers like the microsoft c++ compiler provide such a flag too.

-- 
Kind Regards
Benjamin Thaut

On Thursday, 10 January 2013 at 10:18:38 UTC, Benjamin Thaut wrote:
> Am 10.01.2013 09:37, schrieb comco:
>> There is a rationale for such checks, but I argue that the place of
>> these checks is not in the compiler itself, but in a separate tool.
>
> But there are no such tools yet for D.
> Couldn't we add a -analyze flag to the compiler which will do such checks? Other compilers like the microsoft c++ compiler provide such a flag too.

A simple flag just won't cut it, because the state that you want to represent is wider. Some rules are speculative in nature, others can be easily broken if you are using some sort of automatic code generation - for example image a GUI forms visual editor which emits D code. It won't be smart enough to follow all the guidelines of the language, still the emitted source code should be considered fine. You at least will need something like a level or target flag - for me assembly code when writing high level code is unacceptable, but for low-level stuff its a must. So you'll need a separate specification of the kind of analyzing you want for the particular code. You'll want some of the rules to be turned off.
Another thing is that if the effort to create such tool is comparable to the effort of adding an analyze flag to the already existing compiler, then in my opinion creating a tool would be the better option - its more nice and separated this way.