www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Shadowing of members

reply Benjamin Thaut <code benjamin-thaut.de> writes:
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
Jan 09 2013
next sibling parent reply "comco" <void.unsigned gmail.com> writes:
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.
Jan 09 2013
parent reply Jacob Carlborg <doob me.com> writes:
On 2013-01-10 03:57, bearophile wrote:

 Most wise programmers use different argument names like a_,b_ and so on,
 because it's very easy to create bugs in that situation.

If you need a special naming convention for your member variables you have made something wrong when designing the language. -- /Jacob Carlborg
Jan 09 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-01-10 10:49, bearophile wrote:

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

If you get an error if you shadowing a member variable it sounds like you need to start using naming conventions. Someone adds a new variable to a base class and suddenly your code breaks cause it use the same name for a local variable. -- /Jacob Carlborg
Jan 10 2013
prev sibling next sibling parent reply "Maxim Fomin" <maxim maxim-fomin.ru> writes:
On Wednesday, 9 January 2013 at 20:36:16 UTC, Benjamin Thaut 
wrote:
 <skipped>
 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

Because local variable is not a member. However I think this should be posted to Bugzilla as a bug or at least as an enhancement request.
Jan 09 2013
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/9/2013 1:22 PM, Maxim Fomin wrote:
 Because local variable is not a member. However I think this should be posted
to
 Bugzilla as a bug or at least as an enhancement request.

It is not a bug. As for an enhancement request, I think comco has made some strong points against it.
Jan 09 2013
next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, January 09, 2013 23:22:19 Andrej Mitrovic wrote:
 On 1/9/13, Walter Bright <newshound2 digitalmars.com> wrote:
 It is not a bug.

Something related I want to ask you about: struct S { int _m; this(int m) { this._m = _m; // meant "this._m = m;" } } I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407.

Why not just prevent identity assignments altogether? What would be the purpose of even allowing a variable to be assigned itself? I see no reason to single out parameters or member variables for that. - Jonathan M Davis
Jan 09 2013
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/9/2013 2:50 PM, comco wrote:
 I can forsee someone somewhere having a problem with this if he wants to write
a
 general enough templated solution.

You're right that often code that looks wrong written by users, is actually needed by generic code.
Jan 09 2013
parent Walter Bright <newshound2 digitalmars.com> writes:
On 1/9/2013 5:05 PM, Andrej Mitrovic wrote:
 On 1/10/13, Walter Bright <newshound2 digitalmars.com> wrote:
 You're right that often code that looks wrong written by users, is actually
 needed by generic code.

Well like I've said, I'm not thinking of warning on identity assignment for all cases, just this particular case. I just need an Ok/Denied so I know whether to spend any time working on the enhancement.

Denied. I don't see the rationale for in one case but not others.
Jan 09 2013
prev sibling next sibling parent "comco" <void.unsigned gmail.com> writes:
On Wednesday, 9 January 2013 at 22:25:53 UTC, Jonathan M Davis 
wrote:
 On Wednesday, January 09, 2013 23:22:19 Andrej Mitrovic wrote:
 On 1/9/13, Walter Bright <newshound2 digitalmars.com> wrote:
 It is not a bug.

Something related I want to ask you about: struct S { int _m; this(int m) { this._m = _m; // meant "this._m = m;" } } I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407.

Why not just prevent identity assignments altogether? What would be the purpose of even allowing a variable to be assigned itself? I see no reason to single out parameters or member variables for that. - Jonathan M Davis

I can forsee someone somewhere having a problem with this if he wants to write a general enough templated solution. I actually can come up with an example. Recently I wrote a template mixin witch works this way: reassign(a, b, c, ...) -> a = b; b = c; .. Now imagine someone wanting to write a template mixin which given local refs and a premutation, permutes them. If the identity assignment is not allowed, where he will be in a lot of trouble, because he will need to write checks. In general the identity as a concept should not be hard-special-cased. The compiler is free to optimize it. Of course, in "simple-enough-cases" the prevention might be worthwhile. But I still think this is a job of another tool - a static code analyzer.
Jan 09 2013
prev sibling next sibling parent "Mehrdad" <wfunction hotmail.com> writes:
On Wednesday, 9 January 2013 at 22:25:53 UTC, Jonathan M Davis 
wrote:
 On Wednesday, January 09, 2013 23:22:19 Andrej Mitrovic wrote:
 On 1/9/13, Walter Bright <newshound2 digitalmars.com> wrote:
 It is not a bug.

Something related I want to ask you about: struct S { int _m; this(int m) { this._m = _m; // meant "this._m = m;" } } I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407.

Why not just prevent identity assignments altogether? What would be the purpose of even allowing a variable to be assigned itself? I see no reason to single out parameters or member variables for that. - Jonathan M Davis

It's very useful as a no-op when debugging.
Jan 09 2013
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 1/10/13, Walter Bright <newshound2 digitalmars.com> wrote:
 You're right that often code that looks wrong written by users, is actually
 needed by generic code.

Well like I've said, I'm not thinking of warning on identity assignment for all cases, just this particular case. I just need an Ok/Denied so I know whether to spend any time working on the enhancement.
Jan 09 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Jonathan M Davis:

 Why not just prevent identity assignments altogether? What 
 would be the
 purpose of even allowing a variable to be assigned itself?

Despite being sometimes useful in generic code (where?), self-assignment is a common source for bugs. Bye, bearophile
Jan 09 2013
prev sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Walter Bright:

 Denied. I don't see the rationale for in one case but not 
 others.

There is a rationale. In my opinion special cases are bad in a language, unless: - They always produce a compilation failure in a well defined group of cases. (while just giving a different answer in a vaguely defined group of cases is usually not acceptable); - They help the programmer avoid a common bug-prone situation. In D we have already some cases of such special rules, that break uniformity and symmetry to help D programmers write less buggy code. Issue 4407 ( http://d.puremagic.com/issues/show_bug.cgi?id=4407 ) is/was one of my fifteen most important enhancement requests (and it's a small one). Many of those fifteen are little breaking changes. I think the case discussed in Issue 4407 passes both conditions: The cause is defined for class/structs. I think this is a definite enough situation Regarding commonality: - I know two tools that detect this bug, the tool that has already found some (different) bugs in the DMD source code and the Google Web Toolkit. - I have created several bugs because of that, and I am forced to use a coding convention (like adding a trailing _ to argument names in constructors) to avoid them. - I know three D programmers that have had the same problem of mine, Byron Heads, the OP and another person. - The static analysis tool I was referring before has uncovered cases of such bug in well used C++ code. Bye, bearophile
Jan 09 2013
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Jan 09, 2013 at 10:17:00PM +0100, 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.

I used to write code like that too, but then I ran into some nasty ambiguity bugs, and now I'm in favor of prohibiting local variables shadowing class members. It's just *too* easy to make a mistake, and have the code write something to a local variable (which is lost upon scope exit) instead of a class member, or vice versa. Add class template parameters to the mix, and it quickly becomes a minefield. I think this kind of shadowing should not be allowed. Even if there's a protected member of a super-super-super class, I'd rather know about a name conflict than to write code like this: class B { protected int a=123; } class A : B { int f(int b) { //int a; // <--- I forgot to write this line ... a = b + 1; // <--- Oops! ... return a; } } Because I forgot to declare 'a', I end up overwriting a base class member which I didn't intend to change. Not good. (Furthermore, if the class hierarchy is big, I may not find out about this until much later -- I may not even be aware that some superclass declares 'a'.) T -- Without geometry, life would be pointless. -- VS
Jan 09 2013
prev sibling next sibling parent "comco" <void.unsigned gmail.com> writes:
On Wednesday, 9 January 2013 at 21:30:32 UTC, H. S. Teoh wrote:
 Because I forgot to declare 'a', I end up overwriting a base 
 class
 member which I didn't intend to change. Not good. (Furthermore, 
 if the
 class hierarchy is big, I may not find out about this until 
 much later
 -- I may not even be aware that some superclass declares 'a'.)


 T

At least this will be consistent behaviour. If it is an error, then a perfectly fine code now will stop compiling after someone (evil person!) adds a protected member 'i' to one of the super-super-super classes. Now not only my loop will stop to compile, but the inheritance will have the nice side effect of not allowing to use i for iteration in __any__ method. Of course, this is an extreme example, but valid.
Jan 09 2013
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Jan 09, 2013 at 10:59:21PM +0100, comco wrote:
 On Wednesday, 9 January 2013 at 21:30:32 UTC, H. S. Teoh wrote:
Because I forgot to declare 'a', I end up overwriting a base class
member which I didn't intend to change. Not good. (Furthermore, if
the class hierarchy is big, I may not find out about this until much
later -- I may not even be aware that some superclass declares 'a'.)


 At least this will be consistent behaviour. If it is an error, then
 a perfectly fine code now will stop compiling after someone (evil
 person!) adds a protected member 'i' to one of the super-super-super
 classes. Now not only my loop will stop to compile, but the
 inheritance will have the nice side effect of not allowing to use i
 for iteration in __any__ method.
 Of course, this is an extreme example, but valid.

Hmm, you're right. I withdraw my argument. :-P T -- I am not young enough to know everything. -- Oscar Wilde
Jan 09 2013
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 1/9/13, Walter Bright <newshound2 digitalmars.com> wrote:
 It is not a bug.

Something related I want to ask you about: struct S { int _m; this(int m) { this._m = _m; // meant "this._m = m;" } } I'd like to add a warning for identity assignments when it involves a parameter and a field of the aggregate which is the parent of the function. This was filed as http://d.puremagic.com/issues/show_bug.cgi?id=4407. Personally I've had this happen to me quite a few times, and I've seen it reported on IRC by other people. We could add a check when warnings are turned on. Thought I'd get a taste of that "preapproved" tag ;).
Jan 09 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, January 09, 2013 21:36:16 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?

Personally, I would find it very annoying to not be able to shadow member variables, particularly with POD types. Stuff as simple as constructor parameters would become annoying struct S { this(int i) { this.i = i; //this shouldn't produce an error or warning } int i; } Local variables can't be shadowed by local variables, because there's no way to access the variable that's been shadowed. That's the only case where shadowing prevents you from accessing the shadowed variable, and it's the only case that's currently illegal. If you want to avoid this sort of problem, then name your member variables differently (e.g. prepend _ to them). Then it generally doesn't even risk being an issue except with POD types where you don't make the members private (and therefore don't prepend _ to them). - Jonathan M Davis
Jan 09 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
comco:

 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.

That's a nice warning to have, because that's bug-prone code, worth avoiding. Bye, bearophile
Jan 09 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
H. S. Teoh:

 I used to write code like that too, but then I ran into some 
 nasty
 ambiguity bugs, and now I'm in favor of prohibiting local 
 variables
 shadowing class members. It's just *too* easy to make a 
 mistake, and
 have the code write something to a local variable (which is 
 lost upon
 scope exit) instead of a class member, or vice versa.

Most wise programmers use different argument names like a_,b_ and so on, because it's very easy to create bugs in that situation. Bye, bearophile
Jan 09 2013
prev sibling next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, January 10, 2013 03:56:10 bearophile wrote:
 comco:
 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.

That's a nice warning to have, because that's bug-prone code, worth avoiding.

So, basically, you want the compiler to yell at you for picking a bad variable name? There's _nothing_ to warn about in the code above. It's perfectly valid. I could see warning if you did this.a = this.a; or a = a; but this.a = a; is perfectly valid. And it would be _really_ annoying to not be able to give constructor parameters the same names as the member variables that they're used to set. Their names will typically differ be a _ if the member variables are private, but for POD types, they're almost certainly going to be identical, and warning about that would just be stupide. - Jonathan M Davis
Jan 09 2013
next sibling parent Benjamin Thaut <code benjamin-thaut.de> writes:
Am 10.01.2013 04:38, schrieb Jonathan M Davis:
 struct S
 {
   this(int a, int b)
   {
   this.a = a;
   this.b = b;
   }

   int a;
   int b;
 }

Please read my original code example. This is not about parameters to constructors. It is about local function variables that shadow members. Kind Regards Benjamin Thaut -- Kind Regards Benjamin Thaut
Jan 09 2013
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-01-10 04:38, Jonathan M Davis wrote:

 2. Do you honestly think that it's not incredibly common to declare
 constructor parameters to have the same names as the member variables that
 they go with? Who _doesn't_ do it that way? The only thing that would mitigate
 how often it would be warned about would be how many POD types in D get away
 without needing to define constructor, because one is implicitly declared. It's
 incredibly common practice in C++, Java. C#, etc. to name constructor
 parameters after the member variables that they go with. If anything, it's
 rare _not_ to do that (aside from private member variables often having stuff
 like _ or m_ added to them).

I do it all the time. -- /Jacob Carlborg
Jan 09 2013
prev sibling next sibling parent "deadalnix" <deadalnix gmail.com> writes:
On Thursday, 10 January 2013 at 03:09:58 UTC, Jonathan M Davis 
wrote:
 On Thursday, January 10, 2013 03:56:10 bearophile wrote:
 comco:
 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.

That's a nice warning to have, because that's bug-prone code, worth avoiding.

So, basically, you want the compiler to yell at you for picking a bad variable name? There's _nothing_ to warn about in the code above. It's perfectly valid. I could see warning if you did this.a = this.a; or a = a; but this.a = a; is perfectly valid. And it would be _really_ annoying to not be able to give constructor parameters the same names as the member variables that they're used to set. Their names will typically differ be a _ if the member variables are private, but for POD types, they're almost certainly going to be identical, and warning about that would just be stupide. - Jonathan M Davis

This argument can go on and on forever. What about getting some hard data ? We should start to gather data when considering such issue. What about adding the warning in some dmd version and trying it on several codebase to get a good view of the impact of such a change ?
Jan 09 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
deadalnix:

 This argument can go on and on forever. What about getting some 
 hard data ?

 We should start to gather data when considering such issue. 
 What about adding the warning in some dmd version and trying it 
 on several codebase to get a good view of the impact of such a 
 change ?

Good idea. (But some soft data is already present, from some persons that have hit this bug, from my personal diary of such bugs, from two static analysis tools that include logic to spot this case, and from reports of this code spotted in C++ code. This data is enough to not dismiss this enhancement request quickly). Bye, bearophile
Jan 09 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, January 10, 2013 04:24:26 deadalnix wrote:
 This argument can go on and on forever. What about getting some
 hard data ?
 
 We should start to gather data when considering such issue. What
 about adding the warning in some dmd version and trying it on
 several codebase to get a good view of the impact of such a
 change ?

1. Walter has already rejected this idea. 2. Do you honestly think that it's not incredibly common to declare constructor parameters to have the same names as the member variables that they go with? Who _doesn't_ do it that way? The only thing that would mitigate how often it would be warned about would be how many POD types in D get away without needing to define constructor, because one is implicitly declared. It's incredibly common practice in C++, Java. C#, etc. to name constructor parameters after the member variables that they go with. If anything, it's rare _not_ to do that (aside from private member variables often having stuff like _ or m_ added to them). 3. Why on earth would I want the compiler to be warning me about the variable names that I pick? No offense. But that's asinine. There are _no_ bugs in code like struct S { this(int a, int b) { this.a = a; this.b = b; } int a; int b; } There aren't even any _potential_ bugs in that code. It's perfectly sound. Warning about something like a = a; would make some sense. Warning against a perfectly valid assignment or the fact that the variable names happen to be the same is just stupid. Warning about something which isn't a bug or at least almost a guarantee to be a bug is _not_ a valid use of a warning IMHO. You don't have warnings for good or bad practice. You have warnings for likely bugs. And I wouldn't even consider the struct above to be bad practice anyway. - Jonathan M Davis
Jan 09 2013
prev sibling next sibling parent "deadalnix" <deadalnix gmail.com> writes:
On Thursday, 10 January 2013 at 03:38:23 UTC, Jonathan M Davis 
wrote:
 On Thursday, January 10, 2013 04:24:26 deadalnix wrote:
 This argument can go on and on forever. What about getting some
 hard data ?
 
 We should start to gather data when considering such issue. 
 What
 about adding the warning in some dmd version and trying it on
 several codebase to get a good view of the impact of such a
 change ?

1. Walter has already rejected this idea. 2. Do you honestly think that it's not incredibly common to declare constructor parameters to have the same names as the member variables that they go with? Who _doesn't_ do it that way? The only thing that would mitigate how often it would be warned about would be how many POD types in D get away without needing to define constructor, because one is implicitly declared. It's incredibly common practice in C++, Java. C#, etc. to name constructor parameters after the member variables that they go with. If anything, it's rare _not_ to do that (aside from private member variables often having stuff like _ or m_ added to them). 3. Why on earth would I want the compiler to be warning me about the variable names that I pick? No offense. But that's asinine. There are _no_ bugs in code like struct S { this(int a, int b) { this.a = a; this.b = b; } int a; int b; } There aren't even any _potential_ bugs in that code. It's perfectly sound. Warning about something like a = a; would make some sense. Warning against a perfectly valid assignment or the fact that the variable names happen to be the same is just stupid. Warning about something which isn't a bug or at least almost a guarantee to be a bug is _not_ a valid use of a warning IMHO. You don't have warnings for good or bad practice. You have warnings for likely bugs. And I wouldn't even consider the struct above to be bad practice anyway. - Jonathan M Davis

So authority argument worth more than actual data. That is noted.
Jan 09 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Jonathan M Davis:

 1. Walter has already rejected this idea.

Walter opinions needs respect, but: - Walter is far from infallible; - I know many cases where he has changed his mind. I remember some cases where he has had to go back on his decisions; - I was away during the largest part of this discussion, so I didn't have a chance to answer before Walter answer; - I see enough rational arguments for this proposal, to not dismiss this issue so quickly; - This is one of my top fifteen enhancement requests, it is sleeping in Bugzilla for a lot of time, and I have the right to answer in this newsgroup if I want.
 Warning about something which isn't a bug or at least almost a 
 guarantee to be
 a bug is _not_ a valid use of a warning IMHO.

I agree, my original enhancement request was was focused only on situations where there is an actual probable mistake in the code. Sorry for generalizing the case too much. Bye, bearophile
Jan 09 2013
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, January 10, 2013 08:17:22 Benjamin Thaut wrote:
 Am 10.01.2013 04:38, schrieb Jonathan M Davis:
 struct S
 {
 
   this(int a, int b)
   {
   this.a = a;
   this.b = b;
   }
   
   int a;
   int b;
 
 }

Please read my original code example. This is not about parameters to constructors. It is about local function variables that shadow members.

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. 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. - Jonathan M Davis
Jan 09 2013
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, January 10, 2013 08:40:00 Jacob Carlborg wrote:
 On 2013-01-10 03:57, bearophile wrote:
 Most wise programmers use different argument names like a_,b_ and so on,
 because it's very easy to create bugs in that situation.

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

You don't need it. It just makes the code clearer if you do. Certain classes of problems don't generally happen when you do that. But if someone prefers to not name their member variables specially, then they don't have to. Just like someone could prefer to always reference member variables with the this pointer/reference, but it's not required. Out of the two, I think that prepending member variables with _ makes way more sense. But to each their own. The language doesn't force anything on you with regards to variable names save for the fact that you can't use key words for them and the fact that you can't shadow local variables with other local variables. - Jonathan M Davis
Jan 09 2013
prev sibling next sibling parent "deadalnix" <deadalnix gmail.com> writes:
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.
Jan 10 2013
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
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
Jan 10 2013
prev sibling next sibling parent reply "comco" <void.unsigned gmail.com> writes:
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.
Jan 10 2013
next sibling parent reply Benjamin Thaut <code benjamin-thaut.de> writes:
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
Jan 10 2013
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/10/2013 2:18 AM, Benjamin Thaut wrote:
 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.

The history of such flags is miserable. Any diagnostic that gives false positives should not be in the compiler. For example, I've run PVS-Studio over the dmd source. It produces a freakin' blizzard of false positives, where the workaround is very ugly. So, one ignores the false positives. It's not so easy to ignore it if it is in the compiler and your company coding standards require passage with no warning messages.
Jan 10 2013
parent reply Jacob Carlborg <doob me.com> writes:
On 2013-01-16 12:17, mist wrote:

 Yes and this is the very difference between compiler and static analysis
 tool, in my opinion. Compiler should never ever reject valid use cases,
 it does not matter how small chance of meeting those is.

Clang contains a ton of warnings for code that could contain common errors. I mean, there's nothing wrong in having unreferenced variables but there are still compilers that warn about this. Also not that Clang has a static analyzer, in addition to the warnings and errors in the compiler. -- /Jacob Carlborg
Jan 16 2013
next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Wednesday, January 16, 2013 13:53:04 mist wrote:
 Well, that is probably the only thing I hate in Clang :) Also, I
 can't imagine a single valid use case for unreferenced variables
 in C/C++, so it is a bit different.

RAII. - Jonathan M Davis
Jan 16 2013
prev sibling parent "mist" <none none.none> writes:
On Wednesday, 16 January 2013 at 15:49:14 UTC, Jonathan M Davis 
wrote:
 On Wednesday, January 16, 2013 13:53:04 mist wrote:
 Well, that is probably the only thing I hate in Clang :) Also, 
 I
 can't imagine a single valid use case for unreferenced 
 variables
 in C/C++, so it is a bit different.

RAII. - Jonathan M Davis

Have just checked it - clang does not treat variable as unused if it does have use-defined destructor. Which makes sense from my understanding of word "unused".
Jan 16 2013
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-01-10 09:37, comco wrote:

 Which brings me to the question - is there any attempt to build such a
 beast for the D programming language?

No, not that I know about. I think it would require a complete front end to do a proper work. Yet another reason why we want a D front end available as a library. -- /Jacob Carlborg
Jan 10 2013
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 1/10/13 5:46 AM, Jacob Carlborg wrote:
 On 2013-01-10 09:37, comco wrote:

 Which brings me to the question - is there any attempt to build such a
 beast for the D programming language?

No, not that I know about. I think it would require a complete front end to do a proper work. Yet another reason why we want a D front end available as a library.

Would the json output suffice? Andrei
Jan 10 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-01-10 16:52, Andrei Alexandrescu wrote:

 Would the json output suffice?

I have no idea. I would though it needed semantic analyzing to do its work. -- /Jacob Carlborg
Jan 10 2013
prev sibling next sibling parent "mist" <none none.none> writes:
 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.
Jan 10 2013
prev sibling next sibling parent "deadalnix" <deadalnix gmail.com> writes:
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.
Jan 10 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
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
Jan 10 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
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
Jan 10 2013
prev sibling next sibling parent "Dejan Lekic" <dejan.lekic gmail.com> writes:
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;
Jan 10 2013
prev sibling next sibling parent "comco" <void.unsigned gmail.com> writes:
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.
Jan 10 2013
prev sibling next sibling parent "comco" <void.unsigned gmail.com> writes:
On Thursday, 10 January 2013 at 15:52:55 UTC, Andrei Alexandrescu 
wrote:
 On 1/10/13 5:46 AM, Jacob Carlborg wrote:
 On 2013-01-10 09:37, comco wrote:

 Which brings me to the question - is there any attempt to 
 build such a
 beast for the D programming language?

No, not that I know about. I think it would require a complete front end to do a proper work. Yet another reason why we want a D front end available as a library.

Would the json output suffice? Andrei

Why not if it's good enough for ctags.
Jan 10 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
Walter Bright:

 Any diagnostic that gives false positives should not be in the 
 compiler.

I generally agree. So do you think spotting what's discussed here is going to cause many false positives? http://d.puremagic.com/issues/show_bug.cgi?id=4407 Bye, bearophile
Jan 10 2013
prev sibling next sibling parent "mist" <none none.none> writes:
On Thursday, 10 January 2013 at 20:08:36 UTC, bearophile wrote:
 Walter Bright:

 Any diagnostic that gives false positives should not be in the 
 compiler.

I generally agree. So do you think spotting what's discussed here is going to cause many false positives? http://d.puremagic.com/issues/show_bug.cgi?id=4407 Bye, bearophile

Many? Even single possible case is enough. And it has been mentioned already - generic code gen, i.e. permutations.
Jan 11 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
mist:

 Many? Even single possible case is enough. And it has been 
 mentioned already - generic code gen, i.e. permutations.

Even one case is enough to kill it? Do you want to add that case in this ER, for future reference purposes? http://d.puremagic.com/issues/show_bug.cgi?id=4407 Bye, bearophile
Jan 16 2013
prev sibling next sibling parent "mist" <none none.none> writes:
On Wednesday, 16 January 2013 at 08:18:53 UTC, bearophile wrote:
 mist:

 Many? Even single possible case is enough. And it has been 
 mentioned already - generic code gen, i.e. permutations.

Even one case is enough to kill it? Do you want to add that case in this ER, for future reference purposes? http://d.puremagic.com/issues/show_bug.cgi?id=4407 Bye, bearophile

Yes and this is the very difference between compiler and static analysis tool, in my opinion. Compiler should never ever reject valid use cases, it does not matter how small chance of meeting those is. Also I am not Walter and second question has probably been targeted wrong :)
Jan 16 2013
prev sibling next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
mist:

 Yes and this is the very difference between compiler and static 
 analysis tool, in my opinion. Compiler should never ever reject 
 valid use cases, it does not matter how small chance of meeting 
 those is.

In most cases debugging is an inherently statistical thing because large programs always contain many bugs. In programming life you can't work with absolutes, you should look at the probabilities too.
 Also I am not Walter and second question has probably been 
 targeted wrong :)

Walter time is probably better spent elsewhere. If you have understood well what Walter meant, you should be able to write that a counterexample in that enhancement request. Bye, bearophile
Jan 16 2013
prev sibling next sibling parent "Andrey" <andr-sar yandex.ru> writes:
 	class B {
 		protected int a=123;
 	}

 	class A : B {
 		int f(int b) {
 			//int a;	// <--- I forgot to write this line
 			...
 			a = b + 1;	// <---  Oops!
 			...
 			return a;
 		}
 	}

That's why I don't understand, why D allows to refer to member variables and functions without "this". I always use "this" and don't have any problems.
Jan 16 2013
prev sibling next sibling parent "mist" <none none.none> writes:
On Wednesday, 16 January 2013 at 11:50:18 UTC, bearophile wrote:
 mist:

 Yes and this is the very difference between compiler and 
 static analysis tool, in my opinion. Compiler should never 
 ever reject valid use cases, it does not matter how small 
 chance of meeting those is.

In most cases debugging is an inherently statistical thing because large programs always contain many bugs. In programming life you can't work with absolutes, you should look at the probabilities too.

Yes, I fully agree with you. That is way having a solid static analysis tool is must for low-level language. But compiler has different job and I like it UNIX-way.
 Also I am not Walter and second question has probably been 
 targeted wrong :)

Walter time is probably better spent elsewhere. If you have understood well what Walter meant, you should be able to write that a counterexample in that enhancement request.

No idea how well I have understood, but I'll scratch a code example, no problem.
Jan 16 2013
prev sibling next sibling parent "mist" <none none.none> writes:
On Wednesday, 16 January 2013 at 12:05:49 UTC, Jacob Carlborg 
wrote:
 On 2013-01-16 12:17, mist wrote:

 Yes and this is the very difference between compiler and 
 static analysis
 tool, in my opinion. Compiler should never ever reject valid 
 use cases,
 it does not matter how small chance of meeting those is.

Clang contains a ton of warnings for code that could contain common errors. I mean, there's nothing wrong in having unreferenced variables but there are still compilers that warn about this. Also not that Clang has a static analyzer, in addition to the warnings and errors in the compiler.

Well, that is probably the only thing I hate in Clang :) Also, I can't imagine a single valid use case for unreferenced variables in C/C++, so it is a bit different.
Jan 16 2013
prev sibling next sibling parent "David Nadlinger" <see klickverbot.at> writes:
On Wednesday, 16 January 2013 at 12:53:05 UTC, mist wrote:
 On Wednesday, 16 January 2013 at 12:05:49 UTC, Jacob Carlborg 
 wrote:
 Clang contains a ton of warnings for code that could contain 
 common errors. […]

Well, that is probably the only thing I hate in Clang :)

Actually, I found GCC to be much noisier in this (-Wall -Wextra) case; the Clang warnings are mostly of pretty good quality.
 Also, I can't imagine a single valid use case for unreferenced 
 variables in C/C++, so it is a bit different.

Variables only used as parameters to assert(), in release builds. This is somewhat annoying, as there isn't already an #if-block you could move them into, as for other similar cases involving conditional compilation. David
Jan 16 2013
prev sibling next sibling parent "mist" <none none.none> writes:
On Wednesday, 16 January 2013 at 16:58:13 UTC, David Nadlinger 
wrote:
 On Wednesday, 16 January 2013 at 12:53:05 UTC, mist wrote:
 On Wednesday, 16 January 2013 at 12:05:49 UTC, Jacob Carlborg 
 wrote:
 Clang contains a ton of warnings for code that could contain 
 common errors. […]

Well, that is probably the only thing I hate in Clang :)

Actually, I found GCC to be much noisier in this (-Wall -Wextra) case; the Clang warnings are mostly of pretty good quality.
 Also, I can't imagine a single valid use case for unreferenced 
 variables in C/C++, so it is a bit different.

Variables only used as parameters to assert(), in release builds. This is somewhat annoying, as there isn't already an #if-block you could move them into, as for other similar cases involving conditional compilation. David

Ah, good catch, glad I have not yet met such a case, I'd probably have raged a lot :) Well, both Clang and GCC are not perfect here. I have seen several times when -Werror policy got canceled because no acceptable workaround was thought of in time. And my development experience is rather small.
Jan 16 2013
prev sibling next sibling parent "SomeDude" <lovelydear mailmetrash.com> writes:
On Thursday, 10 January 2013 at 03:42:54 UTC, deadalnix wrote:
 So authority argument worth more than actual data. That is 
 noted.

You may argue that it's authority, but unless you love petty arguments, you can't argue that it's not a very well known and very widespread usage practice. No need to collect stats to know that.
Jan 20 2013
prev sibling parent "Michael" <pr m1xa.com> writes:
On Wednesday, 16 January 2013 at 12:23:51 UTC, Andrey wrote:
 	class B {
 		protected int a=123;
 	}

 	class A : B {
 		int f(int b) {
 			//int a;	// <--- I forgot to write this line
 			...
 			a = b + 1;	// <---  Oops!
 			...
 			return a;
 		}
 	}

That's why I don't understand, why D allows to refer to member variables and functions without "this". I always use "this" and don't have any problems.

this.because(this.does(this.not(this.make(this.sense())))); or because().it().is().have().sense(); about subject: name convention - good code.
Jan 20 2013