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 ?

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

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

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.

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

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

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

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

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

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