Re: [lmi] Enabling '-Wshadow'

[Vadim Zeitlin]

On Wed, 27 Sep 2017 23:48:09 +0000 Greg Chicares <address@hidden> wrote:

GC> Another source of gcc '-Wshadow' warnings can be removed by updating
GC>   include/xmlwrapp/node.h, line 165 here:
GC>   https://github.com/vslavik/xmlwrapp/blob/master/include/xmlwrapp/node.h
GC> in this vein:
GC> 
GC> -      explicit text (const char *text) : t(text) {}
GC> +      explicit text (const char *arg) : t(arg) {}
GC> 
GC> using any name except 'text' for the argument.

 Thanks, I've fixed this in xmlwrapp:

https://github.com/vslavik/xmlwrapp/commit/6c3b8a0853b274312859bd13d0f73d6bfa901316

and confirmed that there are no similar warnings in it remaining by
checking that

        $ g++-7 -Wall -Wshadow -fsyntax-only -x c++ include/*/*.h

doesn't give any warnings.


GC> Then we'll just have four '-Wshadow' errors in lmi itself. I've already
GC> tested a fix for the one in 'accountvalue.cpp'. Could I ask you to
GC> propose changes for the three below, because you're more familiar with
GC> these files?
GC> 
GC> /opt/lmi/src/lmi/wx_table_generator.cpp: In constructor 
‘wx_table_generator::wx_table_generator(wxDC&, int, int)’:
GC> /opt/lmi/src/lmi/wx_table_generator.cpp:50:5: error: declaration of ‘dc_’ 
shadows a member of ‘wx_table_generator’ [-Werror=shadow]
GC>      )
GC>      ^
GC> In file included from /opt/lmi/src/lmi/wx_table_generator.cpp:24:0:
GC> /opt/lmi/src/lmi/wx_table_generator.hpp:124:11: note: shadowed declaration 
is here
GC>      wxDC& dc_;
GC>            ^~~

 I've already noticed and fixed this one in the commit

https://github.com/vadz/lmi/commit/a107d92665d83873e7313263486f592149e96f88

which could be cherry-picked to master (but please notice that if it's
really cherry-picked, i.e. applied without changes, git will handle it fine
during the merge, but it will result in conflicts if any changes are made).


GC> /opt/lmi/src/lmi/wx_test_validate_output.cpp: In constructor 
‘{anonymous}::init_test_census(const string&, const 
string&)::change_name_in_cell_dialog::change_name_in_cell_dialog(const 
string&)’:
GC> /opt/lmi/src/lmi/wx_test_validate_output.cpp:161:13: error: declaration of 
‘insured_name’ shadows a member of ‘{anonymous}::init_test_census(const 
string&, const string&)::change_name_in_cell_dialog’ [-Werror=shadow]
GC>              :insured_name(insured_name)
GC>              ^
GC> /opt/lmi/src/lmi/wx_test_validate_output.cpp:184:28: note: shadowed 
declaration is here
GC>          std::string const& insured_name;
GC>                             ^~~~~~~~~~~~

 Here, the member "insured_name" should be renamed to "insured_name_" to
follow lmi naming conventions. Should I make a (trivial) patch for it,
commit the change directly to master or let you do it as part of your
changes?

GC> /opt/lmi/src/lmi/main_wx_test.cpp: In constructor 
‘{anonymous}::application_test::test_descriptor::test_descriptor(wx_base_test_case*)’:
GC> /opt/lmi/src/lmi/main_wx_test.cpp:204:13: error: declaration of ‘test’ 
shadows a member of ‘{anonymous}::application_test::test_descriptor’ 
[-Werror=shadow]
GC>              :test(test)
GC>              ^
GC> /opt/lmi/src/lmi/main_wx_test.cpp:221:28: note: shadowed declaration is here
GC>          wx_base_test_case* test;
GC>                             ^~~~

 We could either do the same thing as above (i.e. rename "test" member to
"test_", and maybe make it private as it doesn't seem to be used anywhere
outside of the class) or just rename the ctor argument to "t" for a minimal
fix. Again, would you prefer me to do this myself and, if so, via a patch
or just direct commit or change this yourself together with the other
changes?


GC> As for the comment above:
GC>   # XMLWRAPP !! '-Wno-deprecated-declarations' needed for auto_ptr
GC> wasn't auto_ptr already removed from xmlwrapp, so that I should
GC> just update to the latest version

 Yes, this was fixed ~1.5 years ago in

https://github.com/vslavik/xmlwrapp/commit/dd6d4eb5951e59af880ab0e0d5f2912d89cae977

GC> (hopefully after you consider the "text (const char *text) : t(text)"
GC> change above)?

 This is already done, but there is actually another problem in xmlwrapp
that I ran into while using it in one of my other projects that I'd really
like to fix: its error reporting is poor to the point of uselessness, e.g.
it currently gives the error message consisting of 4 characters "I/O " if
it fails to load a file, see https://github.com/vslavik/xmlwrapp/pull/39
I'm not sure if this affects lmi, it probably doesn't in normal use, but it
could when something weird happens -- which is, of course, exactly when
you'd like to have a correct error message. At the very least I planned to
test if this bug actually can affect it or not, but I still didn't have
time to do it yet, nor fix it correctly in xmlwrapp.

 All this just to say that I still hope to fix this in xmlwrapp one day and
that it's not impossible that lmi would have to update the version it uses
again then. But it's not a reason to avoid upgrading it now, of course,
especially as it really shouldn't be difficult to do it.

 Regards,
VZ