Re: [boost] Re: Re: Regex Shadow Warnings

From: John Maddock (john_at_[hidden])
Date: 2003-11-19 07:51:45

> > When it is required that code compile without triggering warning X, in
> > effect the C++ language has been changed to no longer allow whatever is
> > triggering X.
> >
> > That's OK, if X is virtually always a programming problem. But if it is
> > sometimes perfectly good code, then I wouldn't want to see that warning
> > enabled.
>
> It seems to me that both the library users and the library implementors
have
> various requirements and preferences, and the two should be insulated from
> one another as much as possible. Here, making -Wshadow usable for clients
> who want it would force Boost developers to avoid shadowing code (or spend
> the energy to work around it with #pragmas). As you pointed out, this
could
> restrict perfectly good code and force workarounds throughout Boost,
> depending on the warning in question. However,
>
> > No. Only enable warnings for conditions which are actually errors the
> > overwhelming majority of the time.
>
> the correctness of that statement depends on context. Warnings that fail
to
> meet that criterion in Boost code may still be beneficial in client code.

Here's my take on the issue based on my experience with Boost.regex:

The regex sources now compile cleanly with -Wshadow, during the fix I found:

A couple of places where third party compiler-workarounds introduced
unneeded local variables, the code was cleaned up marginally by fixing
the -Wshadow warnings.
A couple of places where lack of imagination on variable naming on my part
lead to member function arguments with the same name as class members, the
code is probably cleaner as a result of fixing these.
A lot of "busy body" warnings from the reuse of common scope-variable
names - names of counters (i and j) and characters (c), come in for a lot of
flack here.

The Boost.regex test code still does not compile cleanly with -Wshadow, and
it's not going to:

The concept checking code introduces hundreds of lines of warnings.
Global variables in the test code sources "conflict" with the names of
function parameters - I started fixing these by replacing names like "flags"
with "arg_flags", but in the end I gave up, and actually undid the changes I
had made, because IMO they only served to further obfuscate the code.

So to conclude:

If you now include regex, and no other headers, and don't define any global
variables in your source, you won't see any -Wshadow warnings, otherwise all
bets are off.  In principal it's also impossible to fix Boost code so that
no possible "conflict" can occur.  It's a bit like:

#define flags Ha! Ha! Ha!

will thoroughly mess up the regex lib as well.

I also don't think we can turn these warnings on for Boost test code by
default - at least not without spending an awful lot of time chasing our
tails - however an occasional check for -Wshadow is a good idea, as is
for -W4 with VC++.

John.