Re: [lmi] Clang fixes

[Vadim Zeitlin]

On Fri, 25 Mar 2016 18:26:17 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-03-25 13:48, Vadim Zeitlin wrote:
GC> > On Fri, 25 Mar 2016 13:21:57 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC>   
https://github.com/vadz/lmi/pull/7/commits/4a87eab6bcf6222e6c2bd64644f4a48b219a443b
GC> > GC> | Remove unused progress_meter::display_mode_ member
GC> > GC> | This field is never used and resulted in clang 
-Wunused-private-field warning.
GC> > GC> 
GC> > GC> But it is used, e.g. in 'progress_meter_test.cpp'.
GC> > 
GC> >  Sorry, I must be blind but I don't see how is it used there. It does use
GC> > "display_mode" ctor argument, but it's only used by the derived classes,
GC> > not progress_meter itself.
GC> > 
GC> >  After applying this patch I can still compile and run progress_meter_test
GC> > without errors, what am I missing?
GC> 
GC> /MinGW_/bin/g++  -MMD -MP -MT progress_meter.o -MF progress_meter.d  -c -I 
/lmi/src/lmi -I /lmi/src/lmi/tools/pete-2.1.1 -I 
/opt/lmi/local/lib/wx/include/i686-w64-mingw32-msw-unicode-3.1 -I 
/opt/lmi/local/include/wx-3.1 -I /opt/lmi/third_party/include -I 
/opt/lmi/third_party/src -I /opt/lmi/local/include -I 
/opt/lmi/local/include/libxml2 -DLMI_WX_NEW_USE_SO  -DLIBXML_USE_DLL -DSTRICT   
 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL -D__WXMSW__ -DBOOST_STRICT_CONFIG   
-std=c++11 -pedantic-errors -Werror
GC> -Wall -Wcast-align -Wconversion -Wdeprecated-declarations 
-Wdisabled-optimization -Wimport -Wmultichar -Wpacked -Wpointer-arith 
-Wsign-compare -Wundef -Wwrite-strings  -Wno-long-long -Wctor-dtor-privacy 
-Wdeprecated -Wnon-template-friend -Woverloaded-virtual -Wpmf-conversions 
-Wsynth  -W -Wcast-qual -Wredundant-decls  -Wno-conversion 
-Wno-deprecated-declarations -Wno-parentheses -Wno-unused-local-typedefs 
-Wno-unused-variable      -ggdb -O2    /lmi/src/lmi/progress_meter.cpp 
-oprogress_meter.o
GC> /lmi/src/lmi/progress_meter.cpp:75:25: error: unused parameter 
'display_mode' [-Werror=unused-parameter]
GC>      ,enum_display_mode  display_mode
GC>                          ^
GC> cc1plus.exe: all warnings being treated as errors
GC> /lmi/src/lmi/workhorse.make:778: recipe for target 'progress_meter.o' failed
GC> make[2]: *** [progress_meter.o] Error 1

 Thank you, this made me realize that configure misses -Wextra option
(spelt as old "-W" in the makefiles), which means that -Wunused-parameter
is not enabled for configure builds explaining how I missed it. I've fixed
this now (please see https://github.com/vadz/lmi/pull/14) but, to address
the warning, I'd just suppress the parameter name in this particular ctor
definition, IMO it's the simplest and guaranteed correct way to do it and I
don't see any need for any further changes today, do you?

 Also, a question about -Wextra: why are wx_dependent_objects compiled
without it? With a few trivial changes, I don't see any reason not to use
it for them too... If you'd like to enable it for the makefile builds too,
I could test building using the makefiles after the changes of PR14 are
merged.


GC> However, I was missing something, too: progress_meter::display_mode_
GC> actually is not used, at least not yet. Does this "display mode"
GC> concept belong in the design of the class? If it doesn't, then it
GC> should be eradicated completely from the code and the comments.

 I don't think removing it from the comments would be the right thing to
do. It is used in the derived classes, but they're not defined in the
headers and don't have comments, so it makes sense to document this here.
I'm ambivalent about passing this parameter to the ctor, but there is no
real harm in doing it, so I wouldn't really argue for stopping to do this.
OTOH there is clearly no need to store it in the base class, clang warning
aside, what purpose does an unused variable have? IMNSHO it can only create
confusion.

GC> But I'm not convinced that progress_meter::display_mode_ is fundamentally
GC> a wrong idea, or that it can never be useful, so I'm reluctant to spend the
GC> time and effort to root it out and rewrite the documentation.

 I understand this and don't propose to do it.

GC> Therefore, is there a way to tell Clang that its objection is noted,
GC> but we've decided not to take its advice?

 There are plenty of ways to do it, starting from the direct one of just
disabling this warning with a clang-specific pragma to pretending that we
do use it, as you propose, but I don't like doing this at all. What's the
advantage of keeping this unused variable? And pretending that it is used
by adding an accessor for it when it's not really used would be even worse
than keeping it because it would make it even less clear that it is not,
actually, used. Let's just get rid of this variable -- without touching
anything else -- to not just fix the warning but also make the code simpler
and more clear.

 I've updated the PR 9 with this change, please see

https://github.com/vadz/lmi/pull/9/commits/a77ace84929150b3694e3ad2be9d3b99777a0fc8

and I really hope it can be just applied, this issue is really too
insignificant to complicate it further.


GC> > GC> 
https://github.com/vadz/lmi/pull/7/commits/1951274d946f30d4796c1fa508e8c29544911b67
GC> > GC> | Don't test whether unsigned variables are positive
GC> > GC> | This is useless as the comparison can never be true
GC> > GC> 
GC> > GC>  void MultiDimAxisAnyChoice::SelectionChanged()
GC> > GC>  {
GC> > GC>      unsigned int const sel = GetSelection();
GC> > GC> -    if(!(0 <= sel && sel < axis_.GetCardinality()))
GC> > GC> +    if(sel >= axis_.GetCardinality())
GC> > GC> 
GC> > GC> Given:
GC> > GC>   class MultiDimAxisAnyChoice
GC> > GC>     :public  wxChoice
GC> > GC> the member function GetSelection() must come from wxChoice:
GC> > GC> 
GC> > GC> 
http://docs.wxwidgets.org/trunk/classwx_choice.html#af13b41f102b0a819aead40919c5f4944
GC> > GC>   virtual int wxChoice::GetSelection() const
GC> > GC> 
GC> > GC> so isn't assigning it to an 'unsigned int' the real error?
GC> > 
GC> >  Well, the code here clearly expects the selection to be valid, so I
GC> > thought keeping using "unsigned" for it wasn't really wrong. But 
personally
GC> > I also prefer to use "auto" and then cast to unsigned after testing that
GC> > it's positive. I'll make an updated patch for this.
GC> 
GC> Okay.

 FWIW the updated patch is part of the same PR 9 and can be seen here:

https://github.com/vadz/lmi/pull/9/commits/bbcc5461a95758c5da1cf1b0dcf14a6e7964bb75


GC> I kind of thought you'd say that gnu getopt is perfect because it's plain C

 Sorry but why should it be an advantage? I'd very much prefer a C++
library than C one.

GC> Of course they answer in the negative, but their reasoning is not clear to
GC> me. Why do you say it's definitely better? Does it behave the same way as
GC> getopt?

 No, not at all, but that's the whole point. getopt is cryptic and *very*
error prone as you can easily forget to handle some option or forget to
handle an argument of an option which has one or try to use an argument of
an option which doesn't define any or make just about any other error in
the code handling them. The fact that handling the option is very far away
from defining it doesn't help neither and the fact that you need to
manually iterate over all options is pretty bad on its own. . I can go into
the details, of course, but, to turn the question around, what are the
advantages of getopt? Other than being familiar to a certain (relatively
small) proportion of programmers I really don't see anything good in it, as
much as I like the "Unix way", I just can't find any redeeming qualities in
it. I have a feeling that it might be useless to argue against getopt
because you seem to be saying that you want something that behaves like
getopt -- and, of course, nothing is better than getopt from this point of
view. But just about anything is better from any other points of view, such
as ease of use, readability or (type-)safety. Does this really need a
detailed explanation? I mean, isn't just the hack with "// TRICKY !! Some
long options are aliased to unlikely octal values." enough to recoil from
using getopt in horror?

GC> I certainly don't want to use 'cmake' to build a "library" that
GC> seems to contain only one '.cpp' file.

 Of course there is no need to do it, the file could be easily just
included in our own makefiles (how is this different from getopt?). And
there are other, header-only command line parsing libraries.

GC> > GC> because we already have too many dependencies on boost.)
GC> > 
GC> >  We will have only Boost.Filesystem as soon as I submit all my C++11
GC> > modernizing patches... But as those patches touch a lot of code, they
GC> > depend on all the previous patches, so we're not quite there just yet.
GC> 
GC> If you can lay those previous patches out on github, I can make time
GC> for them this weekend. De-boostifying is a worthwhile goal.

 These patches would result in conflicts with many (maybe all) of my
outstanding patches. I'd really prefer to apply as many of those as
possible (including the not yet submitted big mortality tables patch)
first.

 Thanks in advance,
VZ