Re: [lmi] Clang fixes

[Vadim Zeitlin]

On Fri, 25 Mar 2016 13:21:57 +0000 Greg Chicares <address@hidden> wrote:

GC> Here are some comments on
GC>   https://github.com/vadz/lmi/pull/7/files
GC> (the bulk of which I'm committing without comment).

 Thanks a lot for committing this!

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?


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.

GC> Should we instead change it the following way?
GC> -    unsigned int const sel = GetSelection();
GC> +    auto const sel = GetSelection();
GC> (I'm not sure how to use 'const' with 'auto'.)

 Just like you did.

GC> Is 'unsigned int' an obvious error, or might there have been some
GC> reason to choose that type? What happens if GetSelection() returns
GC> wxNOT_FOUND?

 It is converted to UINT_MAX and so doesn't pass the check comparing it
with cardinality and the code still works. But, again, I agree that it's
not the clearest way to do it.


GC> 
https://github.com/vadz/lmi/pull/7/commits/7a8b165e9e813b01b6a8e9c18a6db9eb93ad0a91
GC> | Add support for clang to configure
GC> 
GC> $ make check_concinnity
GC> File 'configure.ac' contains reserved name '__clang__'.
GC> 
GC> Fixed.

 Oops. Thanks.


GC> 
https://github.com/vadz/lmi/pull/7/commits/1117856aea2c14506327ab2b7dfbe03d92ee527b
GC> | Add extra braces to avoid a "possibly dangling else" warning
GC> 
GC> Okay. I didn't find any other instances of the same problem (Clang should
GC> be able to do that better anyway).

 Yes, after half a dozen times when I was sure I found a bug/problem with
its warnings only to finally realize that I missed something and was wrong,
I gained a healthy respect for it.

GC> But I do see some code-smells, like
GC>   if(...
GC>   else if(...
GC>   // no catch-all 'else' at the end
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...


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.

VZ