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
next sibling parent reply "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
next sibling parent reply "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
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 reply "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
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
next sibling parent reply 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
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 parent reply "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
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 parent reply "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
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
prev sibling next sibling parent reply "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
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
parent reply "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
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 parent reply "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 

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
next sibling parent reply "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

 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
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 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 reply 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
parent reply 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
parent reply "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
parent reply 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
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 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

 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 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 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 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 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
next sibling parent reply "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
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 reply 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
parent reply 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
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 "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 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 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 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 "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 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
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 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 "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
parent reply "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
parent reply "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
parent reply "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
next sibling parent reply "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
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 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
parent reply "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
next sibling parent reply 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
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 "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
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 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
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 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