Boost C++ Libraries: Ticket #9378: g++ 4.7 -Wshadow warnings need attention

Eric Niebler — Tue, 12 Nov 2013 23:43:49 GMT

I appreciate the report. Are any of these actual bugs? That is, is there incorrect code? If not, I'm unlikely to fix these. If you wanted to attach a patch though, I'd be willing to apply it.

Wed, 13 Nov 2013 01:48:25 GMT

Well ii's hard to say if there is a bug in some cases because the intent of the shadowed variable is not always clear to a user. Is there some reason you use shadow variables?

Eric Niebler — Wed, 13 Nov 2013 18:06:14 GMT

I don't "use shadow variables." I use standard C++, or I try to. Is any of my code non-conforming?

Mateusz Loskot — Sat, 30 Nov 2013 10:14:51 GMT

From #9374

This is a standard C++ idiom and does not make sense to change.

... in some places, of course.

rmann@… — Tue, 18 Mar 2014 22:32:34 GMT

I'm not sure I agree it's "standard idiom." Just because it's legal code doesn't mean it's a good idea. It opens you up to subtle programming bugs that can be difficult to spot. We recently found a problem due to shadowing a variable, and so turned on -Wshadow. Now we get hundreds of warnings from Boost (and others) about shadowed variables, making it very difficult to spot our own (potential) errors.

Much better programming style to avoid shadowing all together.

Steven Watanabe — Tue, 18 Mar 2014 22:47:55 GMT

Replying to rmann@…:

Much better programming style to avoid shadowing all together.

I very much disagree. It's completely impractical for us to write code that works around all the quirks of the warnings generated by any compiler with any set of options. I personally find this warning much more annoying than useful.

Steven Watanabe — Tue, 18 Mar 2014 22:49:21 GMT

Replying to rmann@…:

Now we get hundreds of warnings from Boost (and others) about shadowed variables, making it very difficult to spot our own (potential) errors.

-isystem is your friend if you don't want to see warnings in other folks headers.

anonymous — Tue, 18 Mar 2014 22:53:23 GMT

Not sure how you can think that way. This is not a question of "working around the quirks" of compilers, it's about writing good code. As I said, we had an error due to shadowing. Compilers almost universally support warning you about this, because it leads to mistakes. But code that prevents us from taking advantage of the compiler makes it harder for us to write good code.

Boost itself can benefit from not shadowing variables, so I don't understand why you wouldn't be all for it. It's simple: shadowing variables opens you up to potential errors, and not shadowing is trivial.

Steven Watanabe — Wed, 19 Mar 2014 01:37:02 GMT

Replying to anonymous:

But code that prevents us from taking advantage of the compiler makes it harder for us to write good code.

I have no objection to you using this warning for your own code. I almost always end up creating a pair of headers disable_warnings.hpp and enable_warnings.hpp that I use to wrap all the code in a library.

Boost itself can benefit from not shadowing variables,

This is highly doubtful. I've seen too many cases where "fixing" one warning only created a different warning (possibly with another compiler) or worse, introduced a bug because someone was blindly following the compiler's suggestions without fully understanding the consequences of each change. There's really no such thing as a trivial change, and considering the effort required, the chances of introducing new problems, and the likelihood of finding a real bug, I simply don't consider it worthwhile to make these changes in a large existing code base.

rmann@… — Wed, 19 Mar 2014 01:44:24 GMT

Clearly a difference in attitude. A similar bug I wrote against Eigen got a response today that they expect to be shadow-free in their next dot release.

I'm only talking about shadowing, not every possible warning under the sun (although I believe it's generally best to avoid them). Obviously it should be fixed by someone who knows what they're doing, but in this case it should be mechanical (identifying errors, of course, is not. But fixing them such that the code remains equivalent is).

Of course Boost can benefit. Right now, you have no idea if you are inadvertently referencing the wrong variable. If you once forget to properly scope the access to a shadowed variable, you've got a bug. Variables of different name make that more obvious. My personal approach is to modify the name of variables according to their scope (constant, global, static, member, parameter, method-local). Not only does this avoid shadowing (in almost every case you might want to use it; the compiler can catch the rest), it also makes it easy to identify where variables come from, without otherwise redundant use of "this".

disable_warnings.hpp and enable_warnings.hpp is a pretty hacky fix.

Steven Watanabe — Wed, 19 Mar 2014 02:44:08 GMT

Replying to rmann@…:

Obviously it should be fixed by someone who knows what they're doing, but in this case it should be mechanical (identifying errors, of course, is not. But fixing them such that the code remains equivalent is).

Mechanically rewriting the code like this has no benefit to anyone.

Either

It's immediately obvious where each variable comes from, or
It isn't obvious what a name refers to.

If (a) is true, then rewriting the code will not make the code more understandable and in fact using mangled names can make it harder to follow. Otherwise, the more (b) is true, the more likely it is that these mechanical changes will accidentally change the behavior.

Besides which, if all you want to do is tell the compiler to shut up, the safest and most reliable solution is to do exactly that, i.e., #pragma GCC warning ignored -Wshadow.

Of course Boost can benefit. Right now, you have no idea if you are inadvertently referencing the wrong variable.

I beg to differ. I'm not.

disable_warnings.hpp and enable_warnings.hpp is a pretty hacky fix.

I don't see why. It means that library code can be compiled with the warning settings that the library author considers useful, rather than whatever each user wants. Suppressing warnings is often necessary anyway (e.g. C4512: Assignment operator could not be generated.) and these headers are a lot more convenient than writing #ifdefs everywhere.

rmann@… — Wed, 19 Mar 2014 02:55:23 GMT

I mean you don't need any special knowledge of what the code is doing to go through and modify variable names to avoid shadowing, if you understand how the compiler resolves scope. Sure, looking at the code, it's obvious which variable is being referenced. What's not obvious is if that was the intent of the programmer of that code.

Why do you think the warning exists at all? if there is no danger to shadowing variables, why does any compiler anywhere support warning you?

The answer is that there IS a danger in doing so. Explicit naming, on the other hand, reduces the likelihood that one will accidentally reference the wrong variable, since the NAME is different. You won't be thinking "foo" when what you really meant was "mFoo" (or "foo_" or whatever style you choose).

Turning off the warning, by any means is sticking your head in the sand and hoping there's nothing wrong. The headers are hacky because you have to litter your code with them. They're a bandaid on the real problem, which is code full of warnings.

If you want to shadow variables in your own code, that's one thing. But code that's used by thousands of others should be free of warnings in the bulk of the compilers it supports.

I clearly can't convince you of the benefit of having warning-free code, shadow or otherwise. It certainly reduces my confidence in Boost being a robust product.

anonymous — Wed, 19 Mar 2014 02:56:11 GMT

I meant to add, of course it benefits everyone. It helps reduce possible mistakes, and it makes it easier to rely on the compiler to catch potential errors.

Steven Watanabe — Wed, 19 Mar 2014 03:28:39 GMT

Warnings are not a problem in themselves. They are simply the compiler's best attempt to guess where there are problems. Not all warnings are appropriate for all coding styles. Some warnings are usually useful, some are useful to some people, and some are almost never useful. I'll point out that -Wshadow is not included in -Wall or even -Wextra. I'll also say that I have fixed -Wshadow warnings in the past. My experience was that it was extremely tedious and found no bugs whatsoever. I also had to hack the documentation toolchain to prevent doxygen from showing the ugly mangled names. I don't consider -Wshadow to be much more useful than if the compiler picked a random line of code and said, "warning: there is code on this line. There might be a bug here." I use warning suppression headers because I only have a finite amount of time available and I'd rather spend it doing something productive (like improving the test suite) than spend it looking at hundreds of messages that only have an infinitesimal chance of being a real problem.

I am not the maintainer of xpressive, of course. How this is handled is ultimately Eric's decision.

status changed; resolution set

Eric Niebler — Wed, 19 Mar 2014 03:34:28 GMT

status new → closed
resolution → wontfix

I've already made my feelings known. This is not a priority. Closing "won't fix". I'm happy to fix genuine bugs. I'm not spending my time playing whack-a-mole with compiler warnings.

anonymous — Tue, 24 Mar 2015 08:38:59 GMT

I found this thread when trying to solve my many warnings issued by the inclusion of boost in my code.

I strongly disagree with, or even appalled by Steven's and Eric's comments. Reusing the same name for another variable is just asking for problems - even if you change the scope to a deeper level. It also makes the code harder to debug and understand.

anonymous — Tue, 24 Mar 2015 08:39:13 GMT

I found this thread when trying to solve my many warnings issued by the inclusion of boost in my code.

Eric Niebler — Tue, 24 Mar 2015 18:05:04 GMT

Appalled! I find it remarkable that someone who feels so strongly about such a trivial thing feels it's better to complain to the author in a public forum than to prepare a patch.

anonymous — Thu, 09 Jun 2016 17:10:49 GMT

Replying to eric_niebler:

Appalled! I find it remarkable that someone who feels so strongly about such a trivial thing feels it's better to complain to the author in a public forum than to prepare a patch.

Are you saying if I create a patch, you are going to actually fix it? If yes, I am going to do it.