Re: [lmi] shadowing warnings (was: Avoid harmless but annoying arg[cv]...)

[Vadim Zeitlin]

On Sat, 4 Jun 2016 23:44:35 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-06-04 17:31, Vadim Zeitlin wrote:
GC> > On Sat, 4 Jun 2016 17:19:42 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> [...]
GC> 
GC> > I didn't look at this in
GC> > details, but the simplest solution would be to ignore the warnings in 
these
GC> > headers using either -isystem gcc option (probably preferable) or #pragmas
GC> > disabling/reenabling warnings before/after including third party headers.
GC> > Unlike the other recently discussed warnings, I think it would be worth
GC> > enabling this one because -Wshadow does find real problems relatively 
often
GC> > in average C++ code. lmi quality is higher than that, of course, but I
GC> > still think it could be useful even for it.
GC> 
GC> But...if it often finds real problems in application code,

 Note that I wrote "relatively often", not really "often". I.e. this
warning is definitely not useless because it did already allow me to find
some code which didn't do what it was intended to do due to not using the
right variable, but it's not like it happens every day.

GC> then isn't it also likely to find real problems in system code (which
GC> '-isystem' would only mask)?

 There is an obvious argument that we implicitly rely on system code
(including boost, xmlwrapp etc) to be correct, so we don't really care
about warnings in them. And another obvious one that it's better to at
least get a chance to notice problems in part of the code, even if it
doesn't cover 100% of the code ending up compiled in lmi.

 But a less obvious argument is that IME this warnings mostly finds
problems in (non trivial) code, i.e. the implementation part, not simple
inline functions usually found in headers. Of course, templates are an
exception to this rule as they're still found in the headers, but are not
always simple. Still, IMHO it's more important to have this warning for the
.cpp files rather than .hpp ones.

 And as you were eager to try, I decided to check myself just how
difficult would it be to enable it for lmi code. It turned out that there
were not that many occurrences of the warnings and with the changes at
https://github.com/vadz/lmi/pull/40 I can now build using autotools build
system if I run configure with CPPFLAGS="-isystem ..." and
CXXFLAGS=-Wshadow. I am not sure myself if the fixes are really worth it,
and I don't think there were any real problems (the only one which might be
worth more attention is the code using "MaxWD", perhaps it should reuse
SetMaxWD() somehow? But I don't really understand what is going on here).
I could run this through the official build system and enable -Wshadow in
it if you'd like to, please let me know.

 Thanks,
VZ