Re: [lmi] MSVC warnings

[Vadim Zeitlin]

On Mon, 6 Jun 2022 03:37:50 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> Off the top of my head

[...snip my explanation of why this didn't work after just receiving your
    next message...]

 However now I have to another point from that message:

GC> The whole point of
GC>     return (t < 0) ? -static_cast<U>(t) : static_cast<U>(t);
GC> is to return the signed value 't' unchanged if it's positive,
GC> else to return its modular-additive inverse. It's already perfect.

 Sorry, but I have to strongly disagree with the conclusion here. It's not
perfect at all because it's very non-obvious that this works correctly, it
took me quite some time to convince myself of this. A comment would
probably help, but using "~t+1" is completely unambiguous about what it is
doing unlike "-(U)t". As you said in another context, the compiler sees no
ambiguity here, but human beings definitely do.

 We may disable the warning, although I'd really rather avoid doing it
globally because this one has caught unintentional errors many times in the
past, but it won't make this function any more clear.

[back to my original reply]

GC> > But could we perhaps use "~t+1" instead of "-t"
GC> > here? As we're guaranteed 2-complement representation in C++ now, it 
should
GC> > do exactly the same thing even in theory now, and not just in practice.
GC> 
GC> But that's such an abstruse theory. I considered ideas (like this)
GC> that perform a negation by operating on the bits, and thought
GC> them too obscure--even though it's a theorem that they're valid,
GC> they're still jarringly unreadable.

 But this is how 2 complement works. Something else may be simpler, but
doesn't correspond to objective reality, etched down in silicon.

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

 Fortunately for the children they only need to operate with the idealised
Platonic arithmetics and not its imperfect applied implementation. Clearly
the Law of the Negation of the Negation doesn't work for C++ because you
can't negate INT_MIN at all.


GC> >  Except for this warning, there are only a few more:
GC> 
GC> Two of them are like this:
GC> 
GC> > ihs_acctval.cpp(421,5): warning C4805: '==': unsafe mix of type 'bool' and
GC> > type 'double' in operation
GC> > 
GC> >    This could again be fixed by just using 0.0 instead of false.
GC> 
GC> Here's the line in question:
GC>     LMI_ASSERT(false                    == InvariantValues().IsMec   );
GC> I hesitate to change that, because the RHS is semantically boolean,
GC> just as though it were 'IsDead' (even though it's held as a double),

 I've raised several times in the past the question of just why exactly it
is held as a double. I seem to remember that the conclusion was that I
shouldn't return to this question again, but, of course, I've completely
forgotten why :-/

GC> Instead, I first want to ask how this can be called unsafe.
GC> Isn't the comparison perfectly well defined, as follows?
GC>   (0.0 = dbl_prec_value) ? false : true

(sorry for nitpicking, but you definitely meant "==" and not "=" here)

GC> And isn't that its obvious and unambiguous meaning? [conv.bool] says:
GC> | A zero value, null pointer value, or null member pointer value is
GC> | converted to false; any other value is converted to true.
GC> To write "0.0" on the LHS, we have to apply that rule, and we're
GC> challenging the reader to figure that out; neither of those is
GC> very difficult, but compilers do things like this for us so that
GC> we don't have to think about them.

 C++ compilers do many things that they shouldn't do, usually for
compatibility reasons, and this is, IMO, one of them. As much as I actually
like handling integer 0 as false and non-0 integers as true, I understand
that even this is pretty suspicious from the type-safety point of view. But
handling doubles as booleans is too much even for me, it's even worse than
handling doubles as ints (which is something else a C++ compiler will
happily do for you, but I think we both agree that it's a mistake to rely
on this happening).

 Anyhow, there is no doubt that the behaviour of this code is well defined.
But IMNSHO it's also pretty clear that in many cases comparing double with
bool indicates a mistake, when you meant to compare it with something else,
because there is usually no reason at all to compare variables of different
types nor rely on implicit conversions from double to int or bool.

GC> What was the author of this warning thinking? What actual mistake
GC> would this prevent? Perhaps the following?
GC> 
GC>   int x {17};
GC>   if(true == x)       {do_something_if_x_is_true()}
GC>   else if(false == x) {do_something_if_x_is_false()}
GC>   else {}; // Impossible by law of the excluded middle.

 I don't know what they were thinking, but I believe it could well be
something much more basic, e.g.

        void f(double foo, double baz, bool bar)
        {
            if(foo == bar) {} // typo, meant to compare with baz here
        }

 There is also, of course, an even more fundamental question of comparing
floating point numbers for equality being suspicious (and gcc has
-Wfloat-equal for this).

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

 I admit that I don't remember making any actual mistakes that were caught
by this warning, so it indeed doesn't seem to very useful in practice. But
OTOH comparing doubles and booleans very clearly seems like not the right
thing to do.

 To summarize, I'd rather stop using doubles for booleans entirely, but,
even if I don't remember the reasons for it, I think this is impossible to
do in practice. If we can't do it, then I think explicitly casting doubles
to bool is the next best idea, just to show that we really know what we're
doing. But if this is undesirable neither, I can indeed just suppress this
warning globally.


GC> This monstrosity in 'zero.hpp' is an 'f2c' translation from FORTRAN.
GC> 
GC> A header should define extremely few if any global variables, and
GC> none with such short and useful names. I wish I had been aware of
GC> this when I was working with that code, when I might easily have
GC> found a good alternative.
GC> 
GC> >    So I wonder if we could
GC> >    perhaps avoid this by moving c2 and c3 declarations inside rroot_()
GC> >    instead of keeping them in the global namespace?
GC> 
GC> But how can we be sure that's valid, even though they appear to
GC> be used only in that function?

 I've pinched my nose and looked at this code and can confirm that it is
valid because they're only ever read by the function and never modified by
it. So, in fact, both c2 and c3 are really just constants 2 and 3 and you
could easily confirm it by:

1. Changing newqua_() signature to take "int const* k" instead of "int* k".
2. Changing it to "int const& k" and replacing "*k" with "k" in the body
   and passing it "c2" and "c3" instead of "&c2" and "&c3".
3. Changing the signature to "int k" (without any further changes).
4. Replacing "c2" and "c3" in the calling code with "2" and "3".
5. Removing "c2" and "c3" entirely.

Whether you want to do it or not is another question, but each of the steps
above is a purely mechanical transformation not modifying the code
semantics and so this should be enough to prove that "c2" and "c3" are
never changed. And because they never change, it doesn't matter if they're
global or static (or exist at all).

GC> >    Alternatively, we could
GC> >    put them in some lmi_zero namespace or something like (but using 
unnamed
GC> >    namespace wouldn't work, i.e. would still result in this warning).
GC> 
GC> Then we'd have to change all references to, e.g.:
GC>         newqua_(a, b, &d_0, &fa, &fb, &fd, &c0, &some_namespace::c2);

 Yes.

GC> I think it should be safe just to change the names:
GC> 
GC> -static int c2 = 2;
GC> -static int c3 = 3;
GC> +static int global_c2 = 2;
GC> +static int global_c3 = 3;
GC> 
GC> with a comment expressing our aversion.

 This is the same as above and would work too, but the simplest possible
change is IMO better/more in line with the current code style:

---------------------------------- >8 --------------------------------------
diff --git a/zero.hpp b/zero.hpp
index 6f1c481fd..b7a07cf22 100644
--- a/zero.hpp
+++ b/zero.hpp
@@ -1155,9 +1155,6 @@ double brent_zero_reference
 
 /* Table of constant values */
 
-static int c2 = 2;
-static int c3 = 3;
-
 int tole_(double* b, double* tol, int* neps, double* eps);
 int func_(int* /* nprob */, double* x, double* fx);
 template<typename FunctionalType>
@@ -1227,6 +1224,9 @@ template<typename FunctionalType>
 int rroot_(FunctionalType& f, int* nprob, int* neps, double* eps,
         double* a, double* b, double* root, int* n_eval)
 {
+    static int c2 = 2;
+    static int c3 = 3;
+
     /* System generated locals */
     double d_1;
 
---------------------------------- >8 --------------------------------------

 Thanks for looking at this,
VZ

pgpUiGuKe1jN3.pgp
Description: PGP signature