lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] More Clang fixes


From: Greg Chicares
Subject: [lmi] More Clang fixes
Date: Sun, 27 Mar 2016 10:19:47 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.5.0

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

'tier_view_editor.cpp':

-    if(ba.GetMinValue() != 0 || ba.GetMaxValue() < 0)
+    if(ba.GetMinValue() != 0)

I can't easily figure out what type GetMaxValue() returns. I guess Clang is
real smart, so we can probably take its word that the type must be unsigned;
but I feel like I'm shooting in the dark. Maybe you understand this, but I
sure don't, so I'm going to commit

+    if(ba.GetMinValue() != 0 || ba.GetMaxValue() < ba.GetMinValue())

instead and rely on careful testing to see whether that stronger condition
is correct. It seems that ba.GetMaxValue() should be tested somehow, and
if the maximum is less than the minimum then a seemingly obvious invariant
doesn't hold. And, given that GetMinValue() is required to equal zero,
this means the same thing as the original.

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

'multidimgrid_any.cpp':

-    unsigned int const sel = GetSelection();
-    if(!(0 <= sel && sel < axis_.GetCardinality()))
+    auto const sel_orig = GetSelection();
+    if(sel_orig < 0)
+        {
+        fatal_error()
+            << "Choice control has unexpectedly invalid selection."
+            << LMI_FLUSH
+            ;
+        }
+
+    unsigned int const sel = static_cast<unsigned int>(sel_orig);
+    if(sel >= axis_.GetCardinality())

I think this calls
  int wxChoice::GetSelection()
so the fundamental mistake was assigning it to an unsigned local variable.
Suppose we remove that problem directly:

- unsigned int const sel = GetSelection();
+ int const sel = GetSelection();

Then we're still comparing signed and unsigned here:
     if(!(0 <= sel && sel < axis_.GetCardinality()))

                          ^

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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]