Re: [lmi] Clang fixes

[Greg Chicares]

On 2016-03-25 13:48, Vadim Zeitlin wrote:
> On Fri, 25 Mar 2016 13:21:57 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC>   
> https://github.com/vadz/lmi/pull/7/commits/4a87eab6bcf6222e6c2bd64644f4a48b219a443b
> GC> | Remove unused progress_meter::display_mode_ member
> GC> | This field is never used and resulted in clang -Wunused-private-field 
> warning.
> GC> 
> GC> But it is used, e.g. in 'progress_meter_test.cpp'.
> 
>  Sorry, I must be blind but I don't see how is it used there. It does use
> "display_mode" ctor argument, but it's only used by the derived classes,
> not progress_meter itself.
> 
>  After applying this patch I can still compile and run progress_meter_test
> without errors, what am I missing?

/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
-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

/lmi/src/lmi/progress_meter.cpp:75:25: error: unused parameter 'display_mode' 
[-Werror=unused-parameter]

     ,enum_display_mode  display_mode

                         ^

cc1plus.exe: all warnings being treated as errors

/lmi/src/lmi/workhorse.make:778: recipe for target 'progress_meter.o' failed

make[2]: *** [progress_meter.o] Error 1


However, I was missing something, too: progress_meter::display_mode_ actually
is not used, at least not yet. Does this "display mode" concept belong in the
design of the class? If it doesn't, then it should be eradicated completely
from the code and the comments. But I'm not convinced that it doesn't belong:
we might someday want a command-line program with a '--progress' option (like
git-fetch, e.g.), which might share the progressing code with a GUI. Then is
it clearly inappropriate for "display mode" to be a member? Perhaps we can do
without that, given that so far it has been implemented in the derived classes
only. But I'm not convinced that progress_meter::display_mode_ is fundamentally
a wrong idea, or that it can never be useful, so I'm reluctant to spend the
time and effort to root it out and rewrite the documentation.

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

--------8<----------------8<----------------8<----------------8<--------
Index: progress_meter.hpp
===================================================================
--- progress_meter.hpp  (revision 6519)
+++ progress_meter.hpp  (working copy)
@@ -227,6 +227,11 @@

     int count() const;
     int max_count() const;
+    // This accessor is not actually used today; it serves only to
+    // prevent Clang from complaining that the member it accesses is
+    // otherwise unused. See:
+    //   http://lists.nongnu.org/archive/html/lmi/2016-03/msg00035.html
+    enum_display_mode display_mode() const {return display_mode_;}

     virtual void        do_dawdle            (int seconds);
     virtual std::string progress_message     () const = 0;

-------->8---------------->8---------------->8---------------->8--------

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

Okay.

[...getopt...]
> GC> This is from libg++, which was declared obsolete in 1998. Shouldn't we use
> GC> gnu getopt instead? (Not boost::program_options,
> 
>  I wish we didn't use either, I don't even know which one do I dislike
> more. Unfortunately (and incredibly) there is no perfect command line
> parser for C++. Some people like https://github.com/docopt/docopt.cpp and
> it's definitely better than getopt, so if you could be amenable to being
> convinced to use it, please let me know...

I kind of thought you'd say that gnu getopt is perfect because it's plain C
and it's been used so widely for so many decades that it's free of known
defects. Is that not the case? Well...wait, that's what the first paragraph
of the 'docopt' documentation says:

https://github.com/docopt/docopt.cpp/blob/master/README.rst
| These timeless functions have been around for decades and have proven we
| don't need anything better, right?

Of course they answer in the negative, but their reasoning is not clear to
me. Why do you say it's definitely better? Does it behave the same way as
getopt? I certainly don't want to use 'cmake' to build a "library" that
seems to contain only one '.cpp' file.

> GC> because we already have too many dependencies on boost.)
> 
>  We will have only Boost.Filesystem as soon as I submit all my C++11
> modernizing patches... But as those patches touch a lot of code, they
> depend on all the previous patches, so we're not quite there just yet.

If you can lay those previous patches out on github, I can make time
for them this weekend. De-boostifying is a worthwhile goal.