* Lars Viklund [2013-11-09 10:23+0100]

On Fri, Nov 08, 2013 at 06:31:25PM -0600, Tom Browder wrote:

...

I am trying to use Boost 1.54.0 to replace a previous version (1.47) and am using g++ 4.7 with most warnings turned on. I am getting many -Wshadow errors and wonder why they exist. Surely the developers test for such.

Warning-free code is a pipe dream, particularly if you target an open-ended set of compilers and compiler versions.

Sure, it'd be "nice" if there were no warnings from third party code, but it's often detrimental to cover the code in warts that serve no actual purpose but to silence warnings, whether it's casting away return values, SAL-like annotations, etc.

...

Here is one example from xpressive/detail/utility/hash_peek_bitset.hpp:

Both 'icase' and 'count' are reported to shadow members of 'this'.

isn't good practice IMHO to do such so that is why I like to use some

"Good practice" gets you deep into bikeshedding real quick. I'd say it's _more_ bug-prone if an author strays too far from his comfort zone in projects where it's not required.

...

I could file many bugs but it begs the question of why the warnings are allowed in the first place.

Different people have different conventions for things like member functions. In this case, the author seems to be a fan of the "access any members via this" approach, where it's perfectly fine to have shadowing.

If the whole reason for this hubbub is because you're one of those folks that use -Werror, it's your responsibility to tune your set of warnings to rule out the ones that cause excessive false positives.

Warnings are not errors for a reason, they exist as advice that may or may not apply.

Or they may, and any convention for managing shadowing applied to headers would trivially enable a coder to enable shadowing warnings on their own code. This potentially saves them mysterious bugs and sniffing around in a debugger before finding that it was something the compiler could have told them about if it weren't lost in the noise. I agree with not being religious about it, but -Wshadow is a pretty achievable goal.

-- Lars Viklund | zao@acc.umu.se _______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users

-- -ericP office: +1.617.599.3509 mobile: +33.6.80.80.35.59 (eric@w3.org) Feel free to forward this message to any list for any purpose other than email address distribution. There are subtle nuances encoded in font variation and clever layout which can only be seen by printing this message on high-clay paper.

...

...

Warnings are not errors for a reason, they exist as advice that may or may not apply.

And that's why it's important to release warning-free code to outsiders who can't always easily determine the true intent of the original author. I am using Boost to help support the well-respected BRL-CAD package (http://brlcad.org) whose policy is to release warning-free code, so Boost releases being warning-free would be a very good thing.

Indeed, but please bare in mind that GCC may not be the author's normal "everyday" compiler, and since -Wshadow isn't on by default (and isn't warned about by most other compilers) they may simply not be aware of the issue. That and an ever shifting number of warnings when new compiler releases come out. I suggest you file bug reports regarding any problems you find: if the fix is trivial and they come with patches even better. While I try to keep my stuff warning clean with MSVC and GCC, but after than all bets are off. Certainly for compilers like Intel's that output pages and pages of "notes" that aren't even actual warnings :-( John.

On Sat, Nov 9, 2013 at 7:08 AM, Tom Browder wrote:

On Sat, Nov 9, 2013 at 6:46 AM, John Maddock wrote: ...

...

...

warning-free code, so Boost releases being warning-free would be a very good thing. ... I suggest you file bug reports regarding any problems you find: if the fix is trivial and they come with patches even better.

Okay, I've downloaded and installed 1.55.0 and filed my first set of bugs (not all -Wshadow, five or so -Wundefs, too). I will add some patches later (for the ones I think I can figure out), but quite a few are head-scratchers for me--that's why I wish you had a policy of checking with more warnings for releases. Ref the -Wshadow warning in particular: IMHO, the obstinate and continued use of a shadowed name (just because one thinks one can get away with it) in a well-respected library like Boost is not good for business nor the author's reputation. Best regards, -Tom

On Tue, Nov 12, 2013 at 8:20 PM, Edward Diener wrote:

On 11/12/2013 7:03 PM, Tom Browder wrote:

...

On Sat, Nov 9, 2013 at 7:08 AM, Tom Browder wrote:

...

On Sat, Nov 9, 2013 at 6:46 AM, John Maddock wrote:

...

I suggest you file bug reports regarding any problems you find: if the fix is trivial and they come with patches even better. ... Ref the -Wshadow warning in particular: IMHO, the obstinate and continued use of a shadowed name (just because one thinks one can get away with it) in a well-respected library like Boost is not good for business nor the author's reputation. ... The last sentence is a pretty personal comment.

There is absolutely nothing in the C++ standard which prevents the situation to which you object. Why therefore bring in judgments like "obstinate", "can

Yes, there is no C++ standard objection to obfuscated code, either. I'm just saying the Boost headers are essentially public APIs and are very intrusive when the user turns on warnings and they light up like a Christmas tree--not good for business or the user's confidence. Best regards, -Tom

On 12 Nov 2013 at 18:03, Tom Browder wrote:

Ref the -Wshadow warning in particular: IMHO, the obstinate and continued use of a shadowed name (just because one thinks one can get away with it) in a well-respected library like Boost is not good for business nor the author's reputation.

I personally deliberately and intentionally shadow both variable and type names in non-public namespaces where doing so significantly eases my maintenance costs. One particular area where this can be needed is getting some code to compile across differing levels of native STL C++ 11 compliance where you want either Boost's or the STL's implementation to get selected. None of this pollutes any public namespace - I therefore see no reason why I should be stopped from making my life easier. Bugs in my use of shadowing are entirely my problem, not someone else's. Niall -- Currently unemployed and looking for work. Work Portfolio: http://careers.stackoverflow.com/nialldouglas/

On 13 Nov 2013 at 12:32, Tom Browder wrote:

...

I personally deliberately and intentionally shadow both variable and type names in non-public namespaces where doing so significantly eases my maintenance costs.

I can't address how shadowing eases your maintenance costs (but I would be interested to hear how).

Here you go: https://github.com/BoostGSoC/boost.afio/blob/master/boost/afio/detail/ std_atomic_mutex_chrono.hpp This technique is not unusual in Boost: mapping C++11 STL facilities into a namespace, or if not available then Boost equivalents. AFIO code can then use boost::afio::future not caring who implemented it, and that is a serious maintenance win. This of course explicitly makes problematic someone who does using namespace boost::afio to map afio's stuff into the local namespace - in particular, doing using namespace boost::afio in the global namespace will cause the compiler to fail to compile due to symbol collision, while mapping afio's namespace into a local namespace also containing a map of boost and/or std will cause a compile error at the point of ambiguous symbol use. I expect that will come up in peer review, and maybe I'll be asked to move these implementation maps into a boost::afio::impl_maps namespace or something to prevent collision with namespace boost or namespace std. Equally people really ought to not map any namespace into the global namespace, it's always a bad idea so a compile error is a good thing. And for local namespace maps you can resolve ambiguous symbol clashes using the using keyword.

...

None of this pollutes any public namespace - I therefore see no

Well, Niall, I disagree. Your headers are very much in the user's space and any problems are also the user's problems.

The same goes with anyone's code. If my code has a bug, that's a problem for the user too. I see no difference between that and any shadowing issues if and only if I don't do shadowing in a namespace which imposes upon user code. Shadowing ought to be an implementation detail and/or technique only. If it has no consequence on user code apart from -Wshadow warnings, I see no issue here. Note I am generally opposed to loop variable shadowing done out of laziness, mainly because it makes printing the current variable value during debugging hard. That said, I have been known to occasionally do so anyway e.g. in macro expansion tricks. And member variable shadowing e.g. where you intentionally shadow a this->variable with variable is a useful technique to (sparingly) have to hand. Niall -- Currently unemployed and looking for work. Work Portfolio: http://careers.stackoverflow.com/nialldouglas/

On Wed, Nov 13, 2013 at 8:06 AM, Jeff Flinn wrote:

Maybe I don't see all of the context but how does icase shadow icase_ in the code above? Also std::size_t count = this->bset_.count(); seems perfectly fine to me - less of an issue than picking a less suitable variable name.

Note that the class has both an icase() and count() member so g++ is warning about the possible confusion of identical class member names. Remember, the intent of the warning is to warn the programmer about possible misuse of one versus the other, so clearly renaming one is a good practice for code for public use. Best regards, -Tom

...

Remember, the intent of the warning is to warn the programmer about possible misuse of one versus the other, so clearly renaming one is a good practice for code for public use.

In my opinion this is an over-simplification. Many warnings raise false alarms in perfectly legitimate code and in my professional experience, jumping through the 'warning free' hoops to clean that up leads to obfuscated code as you make things more complicated just to silence warnings.

This is not a good thing.

So, though I don't think the code should be changed to fix this, it could be argued that the warning should be silenced in the header so as to not

On Nov 13, 2013 8:40 PM, "Chris Glover" wrote: pollute your warning stream. That would meet the use case of the programmer writing portable code and counting on -Wshadow. While he/she could put in a zillion pragmas around the includes, maintaining those pragmas makes using the library less attractive. I quit using a library for exactly this reason.

-- chris

_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users