Re: [lmi] MSVC warnings

[Greg Chicares]

On 6/5/22 15:22, Vadim Zeitlin wrote:
[...]
> math_functions.hpp(149,1): warning C4146: unary minus operator applied to
> unsigned type, result still unsigned

The logic of that message does seem compelling.

> (line 149 in b6916ab8e (Avoid deprecated enum conversions, 2022-06-03) that
> I am testing this with is the last line of the u_abs() function) and I
> think it's a pretty useful warning that it would be a pity to disable
> because it I already wrote incorrect code that was flagged by this warning
> even if in this particular case it's almost certainly done intentionally
> and works as intended.

Off the top of my head (I can't work seriously on this yet, because
I have some UBSAN patches to untangle first):

     using U = std::make_unsigned_t<T>;
+    using T = std::make_signed_t<U>;

(the return type is 'T', so the return value is implicitly
cast to 'T' anyway)

-    return (t < 0) ? -static_cast<U>(t) : static_cast<U>(t);
+    return
+          (t < 0)
+        ? -static_cast<T>(static_cast<U>(t))
+        :  static_cast<T>(static_cast<U>(t))
+        ;

The idea is to cast back to a signed type before negating it.

Probably this can be simplified, and perhaps drastically:

-    return (t < 0) ? -static_cast<U>(t) : static_cast<U>(t);
+    return (t < 0) ? -static_cast<T>(static_cast<U>(t)) : t;

I think I like that. How about you?

> But could we perhaps use "~t+1" instead of "-t"
> here? As we're guaranteed 2-complement representation in C++ now, it should
> do exactly the same thing even in theory now, and not just in practice.

But that's such an abstruse theory. I considered ideas (like this)
that perform a negation by operating on the bits, and thought
them too obscure--even though it's a theorem that they're valid,
they're still jarringly unreadable.

And of course the approach in HEAD is just the Law of the Negation
of the Negation, "which every child can understand as soon as the
mysterious junk in which the old idealistic philosophy wrapped
itself is stripped off." [Herrn Eugen Dührings Umwälzung der
Wissenschaft, Marx Engels Werke, Berlin: Dietz Verlag, 1956--,
vol. 20, p. 126.]

>  Except for this warning, there are only a few more:

Two of them are like this:

> ihs_acctval.cpp(421,5): warning C4805: '==': unsafe mix of type 'bool' and
> type 'double' in operation
> 
>    This could again be fixed by just using 0.0 instead of false.

Here's the line in question:
    LMI_ASSERT(false                    == InvariantValues().IsMec   );
I hesitate to change that, because the RHS is semantically boolean,
just as though it were 'IsDead' (even though it's held as a double),
and 'false' on the LHS is the most natural way to write that value.
And I'd also hesitate to introduce a cast here--or maybe two, if
casting directly from double to boolean violates some other rule.

Instead, I first want to ask how this can be called unsafe.
Isn't the comparison perfectly well defined, as follows?
  (0.0 = dbl_prec_value) ? false : true
And isn't that its obvious and unambiguous meaning? [conv.bool] says:
| A zero value, null pointer value, or null member pointer value is
| converted to false; any other value is converted to true.
To write "0.0" on the LHS, we have to apply that rule, and we're
challenging the reader to figure that out; neither of those is
very difficult, but compilers do things like this for us so that
we don't have to think about them.

Of course, we could avoid writing any LHS at all:

- if(false == boolean_condition)
+ if(!boolean_condition)

which is probably a good general guideline; but in this case, it
would destroy the nice symmetry here:

    LMI_ASSERT(11                       == VariantValues().LapseMonth);
    LMI_ASSERT(BasicValues::GetLength() == VariantValues().LapseYear );
-   LMI_ASSERT(false                    == InvariantValues().IsMec   );
+   LMI_ASSERT(                           !InvariantValues().IsMec   );
    LMI_ASSERT(11                       == InvariantValues().MecMonth);
    LMI_ASSERT(BasicValues::GetLength() == InvariantValues().MecYear );

and, who knows, maybe some compiler author would protest that
we've "unsafely" applied a boolean operator to a floating value.

What was the author of this warning thinking? What actual mistake
would this prevent? Perhaps the following?

  int x {17};
  if(true == x)       {do_something_if_x_is_true()}
  else if(false == x) {do_something_if_x_is_false()}
  else {}; // Impossible by law of the excluded middle.

If we were to change this, we'd want to reconsider each instance of:
  git grep 'false.*=='
  git grep 'true.*=='
But I'm present inclined to think that this warning is worse than
useless, and should be suppressed.

> 3. Finally, there is also a warning given inside wx headers when building
>    lmi
> 
> wxWidgets\include\wx\unichar.h(396,55): warning C4459: declaration of 'c2' 
> hides global declaration
> zero.hpp(1158,12): message : see declaration of 'c2'
> 
>    I guess it's really wx's fault for using "c2" as the name of a parameter
>    in this function:
> 
> inline int operator-(char c1, const wxUniCharRef& c2) { return -(c2 - c1); }

It should be perfectly all right to use that as a parameter name.

This monstrosity in 'zero.hpp' is an 'f2c' translation from FORTRAN.

A header should define extremely few if any global variables, and
none with such short and useful names. I wish I had been aware of
this when I was working with that code, when I might easily have
found a good alternative.

>    So I wonder if we could
>    perhaps avoid this by moving c2 and c3 declarations inside rroot_()
>    instead of keeping them in the global namespace?

But how can we be sure that's valid, even though they appear to
be used only in that function?

>    Alternatively, we could
>    put them in some lmi_zero namespace or something like (but using unnamed
>    namespace wouldn't work, i.e. would still result in this warning).

Then we'd have to change all references to, e.g.:
        newqua_(a, b, &d_0, &fa, &fb, &fd, &c0, &some_namespace::c2);

I think it should be safe just to change the names:

-static int c2 = 2;
-static int c3 = 3;
+static int global_c2 = 2;
+static int global_c3 = 3;

with a comment expressing our aversion.

What do you think?