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

[Greg Chicares]

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

I don't understand this one. The only full-word matches for 'oss' in that
file are in these consecutive lines:

    std::ostringstream oss;
    oss
        << "Irrevocably delete "
        << n_sel_items
        << " of "
        << n_items
        << " cells?"
        ;
    int yes_or_no = wxMessageBox
        (oss.str()

and all three occurrences of 'oss' refer to the same object AFAICT.
What am I missing?

> dbdict.cpp(1114): warning C4456: declaration of 'i' hides previous local 
> declaration
>          dbdict.cpp(1093): note: see declaration of 'i'

    for(; i != end_i; ++i)
        {
...code that uses that 'i'...
        for(svci i = z.member_names().begin(); i != z.member_names().end(); 
++i) // JJJ
            {
            z.datum(*i).write(os); // JJJ
            }
        }

On the two lines marked "// JJJ" above, we should replace the symbol 'i' with 
'j'.

> group_values.cpp(366): warning C4456: declaration of 'meter' hides previous 
> local declaration
>          group_values.cpp(249): note: see declaration of 'meter'

One's for
  "Initializing all cells"
and the other is for
  mc_str(*run_basis)
. They're for distinct loops, at different levels. Hmmm. I think I wrestled
with this a while ago. Can the first 'meter':

    Timer timer;
    census_run_result result;
    boost::shared_ptr<progress_meter> meter
        (create_progress_meter
            (cells.size()
            ,"Calculating all cells"
            ,progress_meter_mode(emission)
            )
        );

    ledger_emitter emitter(file, emission);
    result.seconds_for_output_ += emitter.initiate();

be moved after the last two lines quoted immediately above, and then
enclosed with an open brace on the line preceding it and a close brace
on the line following its culmination? I.e.:

    ledger_emitter emitter(file, emission);
    result.seconds_for_output_ += emitter.initiate();

+   // {block to localize 'meter'}
    {
    boost::shared_ptr<progress_meter> meter
        (create_progress_meter
            (cells.size()
            ,"Calculating all cells"
            ,progress_meter_mode(emission)
            )
        );

    for(unsigned int j = 0; j < cells.size(); ++j)
        {
...
        }
    meter->culminate();
    } // {end block to localize 'meter'}

> ihs_acctval.cpp(1205): warning C4458: declaration of 'mode' hides class member
>          account_value.hpp(463): note: see declaration of 
> 'AccountValue::mode' (compiling source file ihs_acctval.cpp)
> ihs_avmly.cpp(701): warning C4458: declaration of 'mode' hides class member
>          account_value.hpp(463): note: see declaration of 
> 'AccountValue::mode' (compiling source file ihs_avmly.cpp)

The problem is in the header:

    // Intermediate values within annual or monthly loop only.
    double      pmt;       // Antediluvian.
    mcenum_mode mode;      // Antediluvian.

Some day we should merge the antediluvian branch; but I've been wanting
to do that for as long as you've wanted to upgrade the compiler. Here,
I'd welcome a change that replaces 'mode' with another name in the
header and in any antediluvian code that uses it. I'd suggest using a
name like 'pmt_mode', 'payment_mode', 'PaymentMode', or 'pmt_mode': I'd
hope that at least one of those is not presently used.

I don't want to change the name in the two places msvc warns about, e.g.:

double AccountValue::SuppositiveModalPremium
    (int         year
    ,mcenum_mode mode
    ,double      specamt

because there (in production code) the name 'mode' is absolutely perfect.

> ledger_xml_io.cpp(764): warning C4456: declaration of 'suffix' hides previous 
> local declaration
>          ledger_xml_io.cpp(726): note: see declaration of 'suffix'

How about enclosing the higher-level declaration (and the code that uses
it) in braces? I.e.:

+   // {block to localize 'suffix'}
+   {
    std::string suffix = "";
    for
        (scalar_map::const_iterator j = scalars.begin()
...
        if(format_exists(j->first, suffix, format_map))
            stringvectors[j->first + suffix] = ledger_format(*j->second, 
format_map[j->first]);
        }
+   }

It looks like 'suffix' is a really good name for what seems to be the same
thing in two different contexts; that's why I think I'd rather localize it
in both, instead of changing the name in either.

> ledger_xml_io.cpp(948): warning C4456: declaration of 'i' hides previous 
> local declaration
>          ledger_xml_io.cpp(761): note: see declaration of 'i'

Throughout this set of contiguous lines:

        for
            (std::map<std::string,std::vector<std::string> >::const_iterator j 
= stringvectors.begin()
            ;j != stringvectors.end()
            ;++j
            )
            {
            ofs << j->first << '\t';
            }
        ofs << '\n';

        for(unsigned int i = 0; i < static_cast<unsigned int>(GetMaxLength()); 
++i)
            {
            for
                (std::map<std::string,std::vector<std::string> 
>::const_iterator j = stringvectors.begin()
                ;j != stringvectors.end()
                ;++j
                )
                {
                std::vector<std::string> const& v = j->second;
                if(i < v.size())
                    {
                    ofs << v[i] << '\t';
                    }
                else
                    {
                    ofs << '\t';
                    }
                }
            ofs << '\n';
            }

I wouldn't mind substituting
  j -> k
  i -> j
if that works. Throughout that whole section, to preserve the existing
parallelism.

> xml_lmi.cpp(245): warning C4456: declaration of 'i' hides previous local 
> declaration
>          xml_lmi.cpp(244): note: see declaration of 'i'

void xml_lmi::xml_document::add_comment(std::string const& s)
{
    xml::node::iterator i = document_->begin();
    for(xml::node::iterator i = document_->begin(); i != document_->end(); ++i)

Here, it seems pretty clear that the first line in the function body
is a pleonastic artifact of an ancient refactoring, and that that line
should just be removed. That's what I did in this very similar case:

http://svn.savannah.nongnu.org/viewvc/lmi/trunk/skeleton.cpp?root=lmi&r1=6443&r2=6489

>  I'd like to fix them if you don't object because I think it's preferable
> to disabling them, especially for the warning about hiding the class member
> which is more error prone than the one about hiding another local variable.

Definitely: 'mode' is such a useful term in the problem domain that
it shouldn't be a member.

> Would you accept patches fixing these warnings? And, if so, what would be
> your preferred way of doing this, especially for the common variable names
> such as "i"? Should I use "i2", "j" or sacrifice brevity and use more
> descriptive names for both the hiding and the hidden variable?

My answers (above) depend on the context of each one. In a couple of
places, I favor adding braces to establish a local scope, where you
might prefer to change the name; please consider humoring me here,
because those areas are complicated and changing a name would make
them harder for me to understand.

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