Re: [lmi] [low priority] How to deal with variable shadowing warnings?

[Vadim Zeitlin]

On Mon, 8 Feb 2016 03:25:06 +0000 Greg Chicares <address@hidden> wrote:

GC> On 02/08/2016 12:54 AM, Vadim Zeitlin wrote:
GC> > 
GC> >  The latest version of MSVC compiler (MSVS 2015 a.k.a. VC 14) enables the
GC> > warnings about variables hiding other names in the same scope by default
GC> > which results in the following warnings (the line numbers are slightly
GC> > different from the mainline as this is from my local branch):
GC> > 
GC> > census_view.cpp(1530): warning C4456: declaration of 'oss' hides previous 
local declaration
GC> >          census_view.cpp(1501): note: see declaration of 'oss'
GC> > census_view.cpp(1536): warning C4456: declaration of 'oss' hides previous 
local declaration
GC> >          census_view.cpp(1501): note: see declaration of 'oss'
GC> 
GC> I don't understand this one.

 The shadowing variable comes from inside LMI_ASSERT() macro. I've renamed
it to something more specific ("assert_message") there to fix this.

GC> > dbdict.cpp(1114): warning C4456: declaration of 'i' hides previous local 
declaration
GC> >          dbdict.cpp(1093): note: see declaration of 'i'
...
GC> On the two lines marked "// JJJ" above, we should replace the symbol
GC> 'i' with 'j'.

 Done.

GC> > group_values.cpp(366): warning C4456: declaration of 'meter' hides 
previous local declaration
GC> >          group_values.cpp(249): note: see declaration of 'meter'
GC> 
GC> One's for
GC>   "Initializing all cells"
GC> and the other is for
GC>   mc_str(*run_basis)
GC> . They're for distinct loops, at different levels. Hmmm. I think I wrestled
GC> with this a while ago. Can the first 'meter':
...
GC> be moved after the last two lines quoted immediately above, and then
GC> enclosed with an open brace on the line preceding it and a close brace
GC> on the line following its culmination?

 This doesn't for several reasons: first, there are several variables
defined below these two lines which are used in the rest of the function.
They could be pulled up, of course, but this already results in a lot of
changes just for a warning fix. But the real problem is the use of "goto"
in this function which would skip over the "meter" dtor if we did the
above, which is, of course, not allowed. This could be changed too, but
this would require more not completely trivial changes.

 And, last but not least, notice that the same "meter" is used in the last
part of the same function for two more unrelated progress meters, so IMO it
doesn't make sense to do something different for one out of 4 meters used
here: either they all should be used once or we can reuse the one which is
currently reused thrice 4 times as well. FWIW my preference would
definitely be to use each object once-only and also use RAII wrappers
calling culminate() automatically on scope exit, but I just don't think
it's worth doing this now.

 So I just reused the same "meter" in all places, after checking that this
doesn't change the existing code behaviour. Hopefully this is acceptable
until the time is found to do something to refactor this function (because
it definitely could profit from it, with its 425 lines of code).

GC> > ihs_acctval.cpp(1205): warning C4458: declaration of 'mode' hides class 
member
GC> >          account_value.hpp(463): note: see declaration of 
'AccountValue::mode' (compiling source file ihs_acctval.cpp)
GC> > ihs_avmly.cpp(701): warning C4458: declaration of 'mode' hides class 
member
GC> >          account_value.hpp(463): note: see declaration of 
'AccountValue::mode' (compiling source file ihs_avmly.cpp)
GC> 
GC> The problem is in the header:
GC> 
GC>     // Intermediate values within annual or monthly loop only.
GC>     double      pmt;       // Antediluvian.
GC>     mcenum_mode mode;      // Antediluvian.
GC> 
GC> Some day we should merge the antediluvian branch; but I've been wanting
GC> to do that for as long as you've wanted to upgrade the compiler. Here,
GC> I'd welcome a change that replaces 'mode' with another name in the
GC> header and in any antediluvian code that uses it. I'd suggest using a
GC> name like 'pmt_mode', 'payment_mode', 'PaymentMode', or 'pmt_mode': I'd
GC> hope that at least one of those is not presently used.

 I took the use of "pmt_mode" as both the first and last suggestion as a
subtle hint that this is the preferred name, so this is what I chose.

GC> > ledger_xml_io.cpp(764): warning C4456: declaration of 'suffix' hides 
previous local declaration
GC> >          ledger_xml_io.cpp(726): note: see declaration of 'suffix'
GC> 
GC> How about enclosing the higher-level declaration (and the code that uses
GC> it) in braces?

 Done.

GC> > ledger_xml_io.cpp(948): warning C4456: declaration of 'i' hides previous 
local declaration
GC> >          ledger_xml_io.cpp(761): note: see declaration of 'i'
GC> 
GC> Throughout this set of contiguous lines:
...
GC> I wouldn't mind substituting
GC>   j -> k
GC>   i -> j
GC> if that works. Throughout that whole section, to preserve the existing
GC> parallelism.

 This would work, but what do you think about my change which simply makes
the first "i" local to the loop it is used in?

GC> > xml_lmi.cpp(245): warning C4456: declaration of 'i' hides previous local 
declaration
GC> >          xml_lmi.cpp(244): note: see declaration of 'i'
GC> 
GC> void xml_lmi::xml_document::add_comment(std::string const& s)
GC> {
GC>     xml::node::iterator i = document_->begin();
GC>     for(xml::node::iterator i = document_->begin(); i != document_->end(); 
++i)
GC> 
GC> Here, it seems pretty clear that the first line in the function body
GC> is a pleonastic artifact of an ancient refactoring, and that that line
GC> should just be removed.

 Yes, done.

GC> I haven't actually tested any of my suggestions above; some of them
GC> might turn out not to be feasible.

 The only one which was problematic was the one concerning the meter. I've
tested the rest of them with both MSVC (of course) and cross-MinGW-w64 and
everything seems fine, so here is the patch.

 Thanks in advance!
VZ

msvc14-shadow-warnings.patch
Description: Text document