Warning policy? local variable hides (i.e. shadows) global variable

older
URGENT: Regression testing repo...

Beman Dawes

15 Jan 2015 15 Jan '15

1:11 p.m.

GCC and Clang have had a warning for years (-Wshadow): "Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed. Note that in C++, the compiler warns if a local variable shadows an explicit typedef, but not if it shadows a struct/class/enum." VC++ 2015 (aka 14.0) Preview has now added a similar warning, C4459. For example, "c:\boost\modular\develop\boost\lexical_cast\detail\converter_lexical_streams.hpp(429): warning C4459: declaration of 'n' hides global declaration". The VC++ warning also provides location info so is easy to view both the declarations involved. In this case, the declaration "bool operator<<(short n)" is shadowing a variable name in the unnamed namespace of the calling translation unit. The process of clearing these shadow warnings occasionally finds bugs that are otherwise difficult to detect. Peter Dimov and I have both found bugs in our code in the process of clearing the new C4459 warning. Even though most of the warnings don't actually signify bugs, I'm finding that clearing them makes code clearer and less confusing, particularly code I haven't looked at in a long while. Should Boost have policy to clear these warnings? A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines: * Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix. Thoughts? --Beman

Show replies by date

Andrey Semashev

15 Jan 15 Jan

1:49 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 4:11 PM, Beman Dawes wrote:

...

GCC and Clang have had a warning for years (-Wshadow): "Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed. Note that in C++, the compiler warns if a local variable shadows an explicit typedef, but not if it shadows a struct/class/enum."

VC++ 2015 (aka 14.0) Preview has now added a similar warning, C4459. For example, "c:\boost\modular\develop\boost\lexical_cast\detail\converter_lexical_streams.hpp(429): warning C4459: declaration of 'n' hides global declaration". The VC++ warning also provides location info so is easy to view both the declarations involved. In this case, the declaration "bool operator<<(short n)" is shadowing a variable name in the unnamed namespace of the calling translation unit.

The process of clearing these shadow warnings occasionally finds bugs that are otherwise difficult to detect. Peter Dimov and I have both found bugs in our code in the process of clearing the new C4459 warning. Even though most of the warnings don't actually signify bugs, I'm finding that clearing them makes code clearer and less confusing, particularly code I haven't looked at in a long while.

Should Boost have policy to clear these warnings?

A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Thoughts?

Despite that the warnings should probably be fixed, I don't think there is need for a naming policy. I'm sure any guideline will contradict someone's habits or preferences, and will be difficult to maintain. Besides it's not clear what to do with tons of written code - starting a new convention without converting the existing code is hardly an improvement. As for the particular issue, it's probably better to mangle the global variable names (my preference is "g_") rather than function arguments or local variables. Globals are much more rare and their names should be chosen carefully anyway.

Rob Stewart

3:19 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On January 15, 2015 8:49:59 AM EST, Andrey Semashev wrote:

...

...
GCC and Clang have had a warning for years (-Wshadow): "Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed."

VC++ 2015 (aka 14.0) Preview has now added a similar warning, C4459.

The process of clearing these shadow warnings occasionally finds bugs that are otherwise difficult to detect. Peter Dimov and I have both found bugs in our code in the process of clearing the new C4459 warning. Even though most of the warnings don't actually signify bugs, I'm finding that clearing them makes code clearer and less confusing, particularly code I haven't looked at in a long while.

Should Boost have policy to clear these warnings?

A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_"

On Thu, Jan 15, 2015 at 4:11 PM, Beman Dawes wrote: prefix

...
for member names has been a success.

Really? I think it's ugly.

...

...
* Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

I use a _ suffix for data members and a _ prefix for formal parameters.

...

...
Thoughts?

Despite that the warnings should probably be fixed, I don't think there is need for a naming policy.

Normally, I'd dismiss such warnings as unwanted noise, but given Beman's anecdotal evidence of the benefit of addressing them, I'm inclined to agree that they should be addressed.

...

I'm sure any guideline will contradict someone's habits or preferences, and will be difficult to maintain.

Exactly

...

As for the particular issue, it's probably better to mangle the global variable names (my preference is "g_") rather than function arguments or local variables. Globals are much more rare and their names should be chosen carefully anyway.

I prefer suffixes for such things, but any convention will work. ___ Rob (Sent from my portable computation engine)

Beman Dawes

5:57 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 10:19 AM, Rob Stewart wrote:

...

...
...
* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success.

Really? I think it's ugly.

Yes, that's why it is a success. It is ugly so users don't want to use it themselves, so it avoids the shadow warning issue.

...

...
...
* Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

I use a _ suffix for data members and a _ prefix for formal parameters.

Yes, those are non-starters because users do follow such conventions.

...

...
...
Thoughts?

Despite that the warnings should probably be fixed, I don't think there is need for a naming policy.

Normally, I'd dismiss such warnings as unwanted noise, but given Beman's anecdotal evidence of the benefit of addressing them, I'm inclined to agree that they should be addressed.

It isn't just me; a quick search finds comments like "The clang -Wshadow warning is quite nice as code which trips it often has bugs due to accidentally referencing the wrong identically named variable" and a complaint Apple defaults to no shadow warnings. --Beman

Rob Stewart

7:43 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On January 15, 2015 12:57:45 PM EST, Beman Dawes wrote:

...

On Thu, Jan 15, 2015 at 10:19 AM, Rob Stewart wrote:

...
...
...
* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success.

Really? I think it's ugly.

Yes, that's why it is a success. It is ugly so users don't want to use it themselves, so it avoids the shadow warning issue.

I guess that's one way of looking at it.

...

...
...
Despite that the warnings should probably be fixed, I don't think there is need for a naming policy.

Normally, I'd dismiss such warnings as unwanted noise, but given Beman's anecdotal evidence of the benefit of addressing them, I'm inclined to agree that they should be addressed.

It isn't just me; a quick search finds comments like "The clang -Wshadow warning is quite nice as code which trips it often has bugs due to accidentally referencing the wrong identically named variable" and a complaint Apple defaults to no shadow warnings.

Interesting ___ Rob (Sent from my portable computation engine)

Stephen Kelly

11:20 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

Beman Dawes wrote:

...

...
Normally, I'd dismiss such warnings as unwanted noise, but given Beman's anecdotal evidence of the benefit of addressing them, I'm inclined to agree that they should be addressed.

It isn't just me; a quick search finds comments like "The clang -Wshadow warning is quite nice as code which trips it often has bugs due to accidentally referencing the wrong identically named variable" and a complaint Apple defaults to no shadow warnings.

I had to laugh that you pasted the text instead of a hyperlink to a location on the information superhighway. :)

Beman Dawes

3:43 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

...

Despite that the warnings should probably be fixed, I don't think there is need for a naming policy. I'm sure any guideline will contradict someone's habits or preferences, and will be difficult to maintain.

The guideline would need to be just that; a guideline that a library maintainer would be free to deviate from. But if there is no guideline at all, then every time a user wants to submit a pull request to fix the shadow warnings, it has to be preceded with a discussion about what to use for the particular library. So at least the community maintained library should try for consistency.

...

Besides it's not clear what to do with tons of written code - starting a new convention without converting the existing code is hardly an improvement.

As for the particular issue, it's probably better to mangle the global variable names (my preference is "g_") rather than function arguments or local variables. Globals are much more rare and their names should be chosen carefully anyway.

The "globals" being shadowed are often in the unnamed namespace in user code. So they involve simple names like "n" and "str" which often clash with parameter names, but the library developer has no control over them. --Beman

Andrey Semashev

4:08 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 6:43 PM, Beman Dawes wrote:

...

...
Despite that the warnings should probably be fixed, I don't think there is need for a naming policy. I'm sure any guideline will contradict someone's habits or preferences, and will be difficult to maintain.

The guideline would need to be just that; a guideline that a library maintainer would be free to deviate from.

But if there is no guideline at all, then every time a user wants to submit a pull request to fix the shadow warnings, it has to be preceded with a discussion about what to use for the particular library.

Right. Each library has its own conventions which have to be learned by contributors, and that won't change whether or not you declare an advisory guideline.

...

So at least the community maintained library should try for consistency.

The CMT is free to agree on such a guideline for libraries it manages, although my point wrt the existing code still stands. IMO incremental code changes should follow naming/formatting rules imposed by the existing code. That is unless these rules are non-existant or absolutely detrimental for maintenance, in which case one should think about a complete rewrite.

...

...
Besides it's not clear what to do with tons of written code - starting a new convention without converting the existing code is hardly an improvement.

As for the particular issue, it's probably better to mangle the global variable names (my preference is "g_") rather than function arguments or local variables. Globals are much more rare and their names should be chosen carefully anyway.

The "globals" being shadowed are often in the unnamed namespace in user code. So they involve simple names like "n" and "str" which often clash with parameter names, but the library developer has no control over them.

But in this case the warning is spurious since the library code uses its parameters in either case. We cannot realistically protect ourselves from such shadowing clashes with the code we don't see, unless we follow STLPort route, which prepends its names with lots of underscores. I say we shouldn't try doing that.

Beman Dawes

5 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 11:08 AM, Andrey Semashev wrote:

...

...
The "globals" being shadowed are often in the unnamed namespace in user code. So they involve simple names like "n" and "str" which often clash with parameter names, but the library developer has no control over them.

But in this case the warning is spurious since the library code uses its parameters in either case.

That's an interesting point. The user can suppress the warning from VC++ by wrapping the header include: #pragma warning (push) #pragma warning (disable : 4459) #include #pragma warning (pop) I suppose GCC has similar pragmas and other compilers support either the MSVC or GCC pragmas. The header itself could provide the pragma's when appropriate (i.e. the library itself not under test). --Beman

...

We cannot realistically protect ourselves from such shadowing clashes with the code we don't see, unless we follow STLPort route, which prepends its names with lots of underscores. I say we shouldn't try doing that.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Paul A. Bristow

5:58 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

...

-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Beman Dawes Sent: 15 January 2015 17:00 To: Boost Developers List Subject: Re: [boost] Warning policy? local variable hides (i.e. shadows) global variable

...
But in this case the warning is spurious since the library code uses its parameters in either case.

That's an interesting point. The user can suppress the warning from VC++ by wrapping the header include:

#pragma warning (push) #pragma warning (disable : 4459) #include #pragma warning (pop)

I suppose GCC has similar pragmas and other compilers support either the MSVC or GCC pragmas.

The header itself could provide the pragma's when appropriate (i.e. the library itself not under test).

We have a document that gives some help on supressing warnings. https://svn.boost.org/trac/boost/wiki/Guidelines/WarningsGuidelines If the warnings are determined by the author to be spurious, then I see reason why using the various mechanisms to suppress the warnings should not be used. Suppressing the warnings documents that the issue has received rational thought and that the warning would be misleading (and often very annoying leading to pages of what is just clutter). (Sadly some platforms have less good mechanisms, but ...) Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830

Niall Douglas

2:25 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On 15 Jan 2015 at 8:11, Beman Dawes wrote:

...

Should Boost have policy to clear these warnings?

Thoughts?

Personally I'd be opposed to setting such a policy for all libraries. Speaking for myself, I intentionally shadow variable names as I find it makes the code clearer e.g. when catching exceptions, I catch to a variable named e. I also, in implementation code, sometimes use macros which hard code the use of certain variable names always being a certain meaning e.g. in AFIO, the size_t variable named id always refers to the "current" op id code, where "current op" can be a bit fuzzy when you're actually dealing with several ops at the time. I have no opposition to each library setting its own policy here. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/

Beman Dawes

5:58 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 9:25 AM, Niall Douglas wrote:

...

On 15 Jan 2015 at 8:11, Beman Dawes wrote:

...
Should Boost have policy to clear these warnings?

Thoughts?

Personally I'd be opposed to setting such a policy for all libraries.

The question needs to be reformulated. We already have a policy to clear warnings in the builds of compiled libraries, and had a sprint a few years ago that eliminated most or maybe even all of those warnings. Where I'm having a problem is headers like lexical_cast.hpp (and its implementation headers) that shadow "n", "str", and "ch". What do we tell users who don't want to turn the warnings off? --Beman

Olaf van der Spek

6:01 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 6:58 PM, Beman Dawes wrote:

...

On Thu, Jan 15, 2015 at 9:25 AM, Niall Douglas wrote:

...
On 15 Jan 2015 at 8:11, Beman Dawes wrote:

...
Should Boost have policy to clear these warnings?

Thoughts?

Personally I'd be opposed to setting such a policy for all libraries.

The question needs to be reformulated. We already have a policy to clear warnings in the builds of compiled libraries, and had a sprint a few years ago that eliminated most or maybe even all of those warnings.

Where I'm having a problem is headers like lexical_cast.hpp (and its implementation headers) that shadow "n", "str", and "ch". What do we tell users who don't want to turn the warnings off?

Don't use global variables (with such generic names). -- Olaf

Rob Stewart

7:45 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On January 15, 2015 12:58:14 PM EST, Beman Dawes wrote:

...

...
On 15 Jan 2015 at 8:11, Beman Dawes wrote:

...
Should Boost have policy to clear these warnings?

Personally I'd be opposed to setting such a policy for all

On Thu, Jan 15, 2015 at 9:25 AM, Niall Douglas wrote: libraries.

The question needs to be reformulated. We already have a policy to clear warnings in the builds of compiled libraries, and had a sprint a few years ago that eliminated most or maybe even all of those warnings.

Where I'm having a problem is headers like lexical_cast.hpp (and its implementation headers) that shadow "n", "str", and "ch". What do we tell users who don't want to turn the warnings off?

It does seem reasonable that Boost software build without -Wshadow warnings. ___ Rob (Sent from my portable computation engine)

Olaf van der Spek

3:09 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 2:11 PM, Beman Dawes wrote:

...

A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Thoughts?

The "_" suffix is used for member names too. -- Olaf

Beman Dawes

3:59 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 10:09 AM, Olaf van der Spek wrote:

...

On Thu, Jan 15, 2015 at 2:11 PM, Beman Dawes wrote:

...
A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Thoughts?

The "_" suffix is used for member names too.

Hummm... You are right. So like with the "m_" prefix, a boost library wishing to avoid shadowing would need to use deliberately ugly or unusual formal parameter names. Ugh! --Beman

Olaf van der Spek

5:22 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Thu, Jan 15, 2015 at 4:59 PM, Beman Dawes wrote:

...

On Thu, Jan 15, 2015 at 10:09 AM, Olaf van der Spek wrote:

...
On Thu, Jan 15, 2015 at 2:11 PM, Beman Dawes wrote:

...
A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Thoughts?

The "_" suffix is used for member names too.

Hummm... You are right.

So like with the "m_" prefix, a boost library wishing to avoid shadowing would need to use deliberately ugly or unusual formal parameter names. Ugh!

Ruby requires @ to access class members. I've been wondering, wouldn't it be nice to have something like this in C++ as well? "m_" and "_" would no longer be required. Obviously this isn't a short-term solution. -- Olaf

Gavin Lambert

16 Jan 16 Jan

3:46 a.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On 16/01/2015 06:22, Olaf van der Spek wrote:

...

On Thu, Jan 15, 2015 at 4:59 PM, Beman Dawes wrote:

...
So like with the "m_" prefix, a boost library wishing to avoid shadowing would need to use deliberately ugly or unusual formal parameter names. Ugh!

Ruby requires @ to access class members. I've been wondering, wouldn't it be nice to have something like this in C++ as well? "m_" and "_" would no longer be required. Obviously this isn't a short-term solution.

There *is* something like that in C++ -- you can prefix all member accesses with "this->". Of course, then you get yelled at by all the people who hate the added verbosity, because this is optional. (And I don't disagree with them; and the pun was intended.) (And it doesn't help with function arguments, which is what was being discussed here.)

Olaf van der Spek

11:19 a.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Fri, Jan 16, 2015 at 4:46 AM, Gavin Lambert wrote:

...

On 16/01/2015 06:22, Olaf van der Spek wrote:

...
On Thu, Jan 15, 2015 at 4:59 PM, Beman Dawes wrote:

...
So like with the "m_" prefix, a boost library wishing to avoid shadowing would need to use deliberately ugly or unusual formal parameter names. Ugh!

Ruby requires @ to access class members. I've been wondering, wouldn't it be nice to have something like this in C++ as well? "m_" and "_" would no longer be required. Obviously this isn't a short-term solution.

There *is* something like that in C++ -- you can prefix all member accesses with "this->".

Right, but that's just ugly. :p

...

Of course, then you get yelled at by all the people who hate the added verbosity, because this is optional. (And I don't disagree with them; and the pun was intended.)

Next step would be to have warnings or maybe a per-class attribute or modifier to make this required.

...

(And it doesn't help with function arguments, which is what was being discussed here.)

Perhaps @@ for non-local stuff? It might look silly at first but I think it's better than relying on coding styles and prefixes. -- Olaf

Andrey Semashev

11:36 a.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Friday 16 January 2015 12:19:30 Olaf van der Spek wrote:

...

On Fri, Jan 16, 2015 at 4:46 AM, Gavin Lambert wrote:

...
On 16/01/2015 06:22, Olaf van der Spek wrote:

...
Ruby requires @ to access class members. I've been wondering, wouldn't it be nice to have something like this in C++ as well? "m_" and "_" would no longer be required. Obviously this isn't a short-term solution.

There *is* something like that in C++ -- you can prefix all member accesses with "this->".

Right, but that's just ugly. :p

...
Of course, then you get yelled at by all the people who hate the added verbosity, because this is optional. (And I don't disagree with them; and the pun was intended.)

Next step would be to have warnings or maybe a per-class attribute or modifier to make this required.

I would really hate that, sorry.

...

...
(And it doesn't help with function arguments, which is what was being discussed here.)

Perhaps @@ for non-local stuff? It might look silly at first but I think it's better than relying on coding styles and prefixes.

Right, let's add some more mangling to the already complicated C++ syntax. I think C++ name lookup rules are very sane and relying on them is the right thing to do. Just pick the right names and you'll be fine. In fact, the proper names will serve as documentation. Warnings about the code that does exactly what is intended is what irritates me in compilers. I often have to cripple the code just to make them happy and not myself. That's why I typically disable such warnings and not "fix" the code. There will always be room for stupid mistakes like "=" instead of "==" in conditions or a ";" right after the "for" operator or @ instead of @@ in your suggested syntax or whatever else. It doesn't mean that the language is broken. It means you have to pay attention.

Olaf van der Spek

18 Jan 18 Jan

4:52 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Fri, Jan 16, 2015 at 12:36 PM, Andrey Semashev wrote:

...

On Friday 16 January 2015 12:19:30 Olaf van der Spek wrote:

...
On Fri, Jan 16, 2015 at 4:46 AM, Gavin Lambert wrote:

...
On 16/01/2015 06:22, Olaf van der Spek wrote:

...
Ruby requires @ to access class members. I've been wondering, wouldn't it be nice to have something like this in C++ as well? "m_" and "_" would no longer be required. Obviously this isn't a short-term solution.

There *is* something like that in C++ -- you can prefix all member accesses with "this->".

Right, but that's just ugly. :p

...
Of course, then you get yelled at by all the people who hate the added verbosity, because this is optional. (And I don't disagree with them; and the pun was intended.)

Next step would be to have warnings or maybe a per-class attribute or modifier to make this required.

I would really hate that, sorry.

Why?

...

...
...
(And it doesn't help with function arguments, which is what was being discussed here.)

Perhaps @@ for non-local stuff? It might look silly at first but I think it's better than relying on coding styles and prefixes.

Right, let's add some more mangling to the already complicated C++ syntax.

I think C++ name lookup rules are very sane and relying on them is the right thing to do. Just pick the right names and you'll be fine. In fact, the proper names will serve as documentation. Warnings about the code that does exactly what is intended is what irritates me in compilers. I often have to cripple the code just to make them happy and not myself. That's why I typically disable such warnings and not "fix" the code.

There will always be room for stupid mistakes like "=" instead of "==" in conditions or a ";" right after the "for" operator or @ instead of @@ in your suggested syntax or whatever else. It doesn't mean that the language is broken. It means you have to pay attention.

Sure, but using a prefix or suffix for member variables is a common practice. Do you not do it? -- Olaf

Andrey Semashev

6:10 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Sunday 18 January 2015 17:52:07 Olaf van der Spek wrote:

...

On Fri, Jan 16, 2015 at 12:36 PM, Andrey Semashev

wrote:

...
On Friday 16 January 2015 12:19:30 Olaf van der Spek wrote:

...
Next step would be to have warnings or maybe a per-class attribute or modifier to make this required.

I would really hate that, sorry.

Why?

The second part of my answer expanded on that, I think.

...

...
...
Perhaps @@ for non-local stuff? It might look silly at first but I think it's better than relying on coding styles and prefixes.

Right, let's add some more mangling to the already complicated C++ syntax.

I think C++ name lookup rules are very sane and relying on them is the right thing to do. Just pick the right names and you'll be fine. In fact, the proper names will serve as documentation. Warnings about the code that does exactly what is intended is what irritates me in compilers. I often have to cripple the code just to make them happy and not myself. That's why I typically disable such warnings and not "fix" the code.

There will always be room for stupid mistakes like "=" instead of "==" in conditions or a ";" right after the "for" operator or @ instead of @@ in your suggested syntax or whatever else. It doesn't mean that the language is broken. It means you have to pay attention.

Sure, but using a prefix or suffix for member variables is a common practice. Do you not do it?

Yes, although not universally. There are cases when no prefixes are needed.

Olaf van der Spek

6:21 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Sun, Jan 18, 2015 at 7:10 PM, Andrey Semashev wrote:

...

...
Sure, but using a prefix or suffix for member variables is a common practice. Do you not do it?

Yes, although not universally. There are cases when no prefixes are needed.

Sure, but why should that stop us from trying to improve the cases where it is needed? -- Olaf

Andrey Semashev

6:45 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On Sun, Jan 18, 2015 at 9:21 PM, Olaf van der Spek wrote:

...

On Sun, Jan 18, 2015 at 7:10 PM, Andrey Semashev wrote:

...
...
Sure, but using a prefix or suffix for member variables is a common practice. Do you not do it?

Yes, although not universally. There are cases when no prefixes are needed.

Sure, but why should that stop us from trying to improve the cases where it is needed?

That's not what you suggested, at least not in that warnings part. As for the @@ syntax or whatever else, I really don't see the necessity in it - prefixes work quite well and don't complicate the language. If you really want to be explicit then this-> is a suitable alternative (and no, I don't think it's ugly - it's just a normal pointer dereference that you use everywhere).

Gavin Lambert

19 Jan 19 Jan

6:04 a.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On 19/01/2015 05:52, Olaf van der Spek wrote:

...

Sure, but using a prefix or suffix for member variables is a common practice. Do you not do it?

Just a reminder that the issue originally being discussed here is where USER CODE has declared a global variable: std::string message; and LIBRARY CODE has declared a method: namespace boost { namespace skywriting { class marquee { public: void set_message(const std::string& message) { // generates warning because local "message" // hides global "message" ... } }; } } This is obviously the compiler being overly pedantic, but the choices seem to be between: 1. Doing nothing, and let the user pick "better" names for their global variables if the warnings offend them (some have suggested using a g_ prefix, but of course the user can do whatever they like). (This may annoy some users who have pedantic warnings on but don't want to "fix" this themselves.) 2. If compiler support exists, disable raising the warning while Boost code is being compiled. (This may hide genuine bugs in Boost that may otherwise be detected when compiling unit tests.) 3. Adopting some weird-and-unlikely-to-collide-with-user naming convention for parameters and other local variables of inline methods. (This will uglify Boost code.) Personally, I would strongly lean towards #1, with the caveat that whatever patterns did lead to discovery of real bugs should probably be identified so they can be acted on. I also wonder if proper precompiled module support would solve this, as that would be a big hint to the compiler that the symbols aren't meant to interact when they're from different modules.

Peter Dimov

15 Jan 15 Jan

3:24 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) globalvariable

Beman Dawes wrote:

...

Should Boost have policy to clear these warnings?

I found one place where -Wshadow was quite annoying. class error { private: std::string name_, reason_; public: error( std::string const & name, std::string const & reason ): name_( name ), reason_( reason ) {} std::string name() const { return name_; } std::string reason() const { return reason_; } } The name() member function is shadowed by the constructor parameter. I can't use name_ for the parameter because of the member. I don't want to use _name for the parameter because the leading underscore is legal but for obscure reasons. So I was forced to use abbreviations like nm and rsn, which isn't very readable.

Beman Dawes

4:34 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) globalvariable

On Thu, Jan 15, 2015 at 10:24 AM, Peter Dimov wrote:

...

Beman Dawes wrote:

...
Should Boost have policy to clear these warnings?

I found one place where -Wshadow was quite annoying.

class error { private:

std::string name_, reason_;

public:

error( std::string const & name, std::string const & reason ): name_( name ), reason_( reason ) {}

std::string name() const { return name_; } std::string reason() const { return reason_; } }

The name() member function is shadowed by the constructor parameter. I can't use name_ for the parameter because of the member. I don't want to use _name for the parameter because the leading underscore is legal but for obscure reasons. So I was forced to use abbreviations like nm and rsn, which isn't very readable.

Agreed. I'm hoping that we can figure some decoration. I've used "name_arg" / "reason_arg" style once in a while, although that is a bit long-winded. --Beman

Bjorn Reese

4:27 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

On 01/15/2015 02:11 PM, Beman Dawes wrote:

...

* Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Projects I have worked on that use this guideline have suffered from subtle errors anyways. As the name with and without the underscore resemble each other too much, these errors tend to go unnoticed, especially if the names are next to operators, e.g. "foo_->bar". I personally avoid shadowing by selecting noticeably different names (usually abbreviations or synonyms) for function arguments.

Robert Ramey

4:47 p.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

If you want to make it a "guideline" I would be OK with that. If you want to make something stronger - I think that this would be a bad idea. The basic problem is that this couples a derived class to it's parent(s) and a library author doesn't always control it's parents: example: template<typename T> struct opaque_type : public T { ... const char *name = "..."; }; struct user_class : public opaque_type<T> { ... // user might really want to shadow const char *name = "asdfasdf"; }; This is a contrived example, so don't call me on this one. My real point is that it's hard to predict the cases where a "hard rule" would be problem. Robert Ramey -- View this message in context: http://boost.2283326.n4.nabble.com/Warning-policy-local-variable-hides-i-e-s... Sent from the Boost - Dev mailing list archive at Nabble.com.

Vicente J. Botet Escriba

19 Jan 19 Jan

7:44 a.m.

New subject: Warning policy? local variable hides (i.e. shadows) global variable

Le 15/01/15 14:11, Beman Dawes a écrit :

...

GCC and Clang have had a warning for years (-Wshadow): "Warn whenever a local variable or type declaration shadows another variable, parameter, type, class member (in C++), or instance variable (in Objective-C) or whenever a built-in function is shadowed. Note that in C++, the compiler warns if a local variable shadows an explicit typedef, but not if it shadows a struct/class/enum."

VC++ 2015 (aka 14.0) Preview has now added a similar warning, C4459. For example, "c:\boost\modular\develop\boost\lexical_cast\detail\converter_lexical_streams.hpp(429): warning C4459: declaration of 'n' hides global declaration". The VC++ warning also provides location info so is easy to view both the declarations involved. In this case, the declaration "bool operator<<(short n)" is shadowing a variable name in the unnamed namespace of the calling translation unit.

The process of clearing these shadow warnings occasionally finds bugs that are otherwise difficult to detect. Peter Dimov and I have both found bugs in our code in the process of clearing the new C4459 warning. Even though most of the warnings don't actually signify bugs, I'm finding that clearing them makes code clearer and less confusing, particularly code I haven't looked at in a long while.

Should Boost have policy to clear these warnings?

A lot of the warnings involve function argument names. Should we have a guideline to prevent shadow warnings? A convention for argument names would make it easier to submit pull requests. Possible guidelines:

* Prefix function argument names with "a_". Rationale: The "m_" prefix for member names has been a success. * Suffix function argument names with "_". Rationale: Short and less distracting than "m_" prefix.

Hi, Boost is a set of libraries without coding style guidelines. Each author has its own rules/guidelines. What is desirable is that at least each source is coherent. I will suggest that each library add in its documentation if it adheres to the no-shadow warnings and if it adheres, how the author want the patches? A global page could be written to this purpose and each library could reference this page and its approach respect to this guidelines. Best, Vicente

3772

Age (days ago)

3776

Last active (days ago)

List overview

Download

29 comments

12 participants

participants (12)

Andrey Semashev
Beman Dawes
Bjorn Reese
Gavin Lambert
Niall Douglas
Olaf van der Spek
Paul A. Bristow
Peter Dimov
Rob Stewart
Robert Ramey
Stephen Kelly
Vicente J. Botet Escriba