Re: [lmi] More Clang fixes

[Vadim Zeitlin]

On Sun, 27 Mar 2016 10:19:47 +0000 Greg Chicares <address@hidden> wrote:

GC> Reexamining this later, 'database_view_editor.cpp' has 'int':
GC>   class DatabaseDurationAxis
GC>     :public AdjustableMaxBoundAxis<int>
GC> where, 'tier_view_editor.hpp' has 'unsigned int':
GC>   class TierBandAxis
GC>     :public AdjustableMaxBoundAxis<unsigned int>
GC> and I suppose that's the underlying design flaw: it should always
GC> be a signed type. Furthermore, it should always be (signed) 'int',
GC> so using a template parameter was a mistake. It still seems more
GC> cautious to commit the more local change above than to redesign it.

<digression-on-the-use-of-unsigned-you-should-feel-free-to-skip>

 I agree with this but I also have to say that I'm not really sure that
using "unsigned" here was a design flaw. Lakos gives, of course, a lot of
very good advices, but the one about not using "unsigned" has never seemed
convincing enough to me. Trusting the summary of his arguments at
http://groups.google.com/group/comp.lang.c++.moderated/msg/849321a0199501e5
(taken from a comment in multidimgrid_tools.hpp), they're rather simple to
refute:

> 1. Using unsigned does not ensure at compile time that negative numbers
> will not be passed at runtime ...

 Of course not, this is a well-known problem of C++ inherited from C:
conventions between numeric types are implicit. There is nothing particular
to unsigned here, ints are also convertible to doubles but this doesn't
mean that we should stop using ints and use doubles evewyehre.

> 1. ... Sure you get warnings for "strlen(myStr) < myInt", but not for int
> x = -1; f(x);

 The warning is useful because it's simple to run into it accidentally. A
warning for f(-1) might be useful too, but even if we could have it, it
would be much less important in practice because I've never seen this
happen accidentally. As usual, the idea is to protect against Murphy, not
against Machiavelli.

> 2. Using unsigned does not allow for the negative value checking without
> first converting back to signed.

 This is neither here nor there because unsigned is only used for values
which are never negative, so you never need to check for this and thus it's
irrelevant that you can't. Equivalently, if you need to check whether a
value is negative, do use signed for it, of course.

> 3. Using unsigned only increases the size of an integer by 1 bit.

 Completely irrelevant, nobody uses unsigned because of this.

> 4. Lakos considers that using unsigned increases the chance of error

 Any studies or empirical observations that Lakos could have based this
conclusion on must have necessarily been done 30+ years ago. Compilers have
changed a lot since then, in particular no compiler gave warnings about
mixing signed and unsigned back then. I feel that these warnings reduce the
chance of an error and I'd really like to have any proof that using
unsigned is deleterious to the code quality in this millennium.

> 5. Using an unsigned interferes with function overloading. If another
> function called f(double) was created with the f(unsigned) above, the
> compiler cannot determine which to use for the statement int x = -1;
> f(x);

 This is an excellent thing, of course. I'd never want the compiler to
silently call neither double nor unsigned overload in this case. IOW, I
have no idea why is it considered to be a problem rather than an advantage.

> 6. Using unsigned can interfere with template instantiation because the
> argument must match exactly. Thus template<unsigned N> requires an
> unsigned. Thus if we had template<unsigned N> class Bits { int _bits:N...
> then Bits<2> would not work because we got an int, not an unsigned. Thus
> we would have to do Bits<unsigned(2)> or some such.

 This is the only item on this list which I can agree is a drawback of
using unsigned as template arguments. However it's a very small one,
because it's literally a one character change ("2" vs "2u").


 So, on balance, I only see one tiny argument not to use unsigned. OTOH
there are two big arguments in favour of using it, of very different
varieties:

1. Theoretic one: it provides more semantic information which is always
   a good thing. It also reduces cognitive dissonance which happens when
   something that obviously can't be negative is represented by a signed
   type. Again, for me this is pretty similar to representing an integer
   by a double: sure, it can be done, but this seems to be obviously wrong.

2. Practical one: unsigned type (size_t) is used everywhere in the standard
   library and if the rest of the code uses signed types, it requires
   converting, implicitly (dangerous; results in warnings) or explicitly
   (annoying; can still be dangerous) back and forth between them.

 IMO either of these arguments alone would be enough. Considering both of
them, I simply don't see how can we even attempt to balance them with a
tiny disadvantageous above.

 Of course, unsigned types in C++ are not perfect, far from it. But not
using them because of this would be akin to saying that C++ type system is
not perfect (which is definitely true), so we should be using "void*" for
everything. We should use at least what the language provides us with.

</digression-on-the-use-of-unsigned-you-should-feel-free-to-skip>

GC> 'multidimgrid_any.cpp':
GC> 
GC> -    unsigned int const sel = GetSelection();
GC> -    if(!(0 <= sel && sel < axis_.GetCardinality()))
GC> +    auto const sel_orig = GetSelection();
GC> +    if(sel_orig < 0)
GC> +        {
GC> +        fatal_error()
GC> +            << "Choice control has unexpectedly invalid selection."
GC> +            << LMI_FLUSH
GC> +            ;
GC> +        }
GC> +
GC> +    unsigned int const sel = static_cast<unsigned int>(sel_orig);
GC> +    if(sel >= axis_.GetCardinality())
GC> 
GC> I think this calls
GC>   int wxChoice::GetSelection()
GC> so the fundamental mistake was assigning it to an unsigned local variable.

 FWIW in my ideal UI library API this function wouldn't return neither
unsigned nor signed int but rather optional<unsigned> value from here.

GC> Suppose we remove that problem directly:
GC> 
GC> - unsigned int const sel = GetSelection();
GC> + int const sel = GetSelection();
GC> 
GC> Then we're still comparing signed and unsigned here:
GC>      if(!(0 <= sel && sel < axis_.GetCardinality()))
GC> 
GC>                           ^
GC> 
GC> because of this:
GC>     virtual unsigned int GetCardinality() const = 0;
GC> (which is why Lakos recommended avoiding 'unsigned' in APIs--and that
GC> applies to the 'tier_view_editor.cpp' case above, too). We're not
GC> going to redesign the API, so this workaround seems best:
GC> -    if(!(0 <= sel && sel < axis_.GetCardinality()))
GC> +    if(!(0 <= sel && sel < static_cast<int>(axis_.GetCardinality())))

 It doesn't matter, of course, but I find it strange that you prefer to
write it like this rather than

         if(!(0 <= sel && static_cast<unsigned>(sel) < axis_.GetCardinality()))

After all -- and even if this is completely theoretic -- the check in the
first branch of "&&" clearly ensures that my cast is correct, while the
cast above might not be necessarily so.

GC> Anyway, I'm going to commit these changes as described above so that I
GC> can move ahead to enhancing the gcc warnings, which requires building
GC> everything from scratch with every 'build_type'.

 Thanks for committing this, this is what counts the most, of course! But
I'd still like to defend the use of unsigned... BTW, the same comment
speaking about removing it refers to "lmi coding standard section 16.13"
but I'm not sure where can I find the lmi coding standard, could you please
point me to it?

 Thanks in advance,
VZ