Re: [lmi] PATCH: tests build fixes for clang

[Vadim Zeitlin]

On Mon, 8 Mar 2021 15:52:07 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 3/7/21 1:10 PM, Vadim Zeitlin wrote:
GC> > 
GC> >  I'd like to submit a series of small fixes to the tests allowing to build
GC> > them with clang, please see 
https://github.com/let-me-illustrate/lmi/pull/173
GC> 
GC> These changes are good:
GC>   6b5cbb99d Disable clang tautological constant compare warning in a test
GC>   975ddded0 Disable clang -Wbraced-scalar-init in a test
GC> but I find the clang warnings confusing.
GC> 
GC> The first may be a matter of terminology. Assigning an integral value V
GC> to an enum variable is
GC>   logically valid for some V, and
GC>   logically invalid for certain other V,
GC> but I'd call it tautologically invalid iff it can be rejected universally
GC> for the category to which V belongs, e.g.:
GC>   oenum_alb_or_anb x;
GC>   x = "fish";
GC> Probably no one else cares about that, but I really did spend time looking
GC> for a deeper, more categorical reason.

 Unfortunately documentation for clang diagnostics is basically
non-existent or, to be precise, it does exist but is perfectly useless.
E.g. here is the documentation for the particular warning that commit
suppressed:

https://clang.llvm.org/docs/DiagnosticsReference.html#wtautological-constant-out-of-range-compare

To compensate for this, the actual warning itself is rather clear, however:

input_test.cpp:164:5: error: result of comparison of constant 25000 with 
expression of type 'oenum_alb_or_anb' is always false 
[-Werror,-Wtautological-constant-out-of-range-compare]
    LMI_TEST_EQUAL(25000, a);
    ^~~~~~~~~~~~~~~~~~~~~~~~

and is, I believe, accurately named: "tautological" refers to the
comparison, which is always false for the values used in it.

 And, naming considerations aside, in practice this warning is useful
because it's not something you typically want to do on purpose, but it's
easy enough to end up with something like this accidentally and it did
happen to me a few times in the past.


GC> The second seems to be misnamed: git-grep easily finds other braced
GC> scalar initializers such as these...
GC> 
GC>   progress_meter.cpp:    :count_         {0}
GC>   rate_table.cpp:    char index_record[e_index_pos_max] = {0};
GC>   round_to.hpp:    int decimals_                    {0};
GC> 
GC> ...but this patch addresses only instances like this:
GC> 
GC>      return {n}; // error: narrowing conversion of '128' from 'unsigned int'
GC> 
GC> ...which is a problem not because it's a braced scalar initializer,
GC> but because the value of 'n' would require narrowing.

 Yes, exactly. This warning documentation is as useless as in the first
case, but here the warning message itself is not good neither as it's just

ssize_lmi_test.cpp:133:12: error: braces around scalar initializer 
[-Werror,-Wbraced-scalar-init]
    return {n}; // error: narrowing conversion of '128' from 'int' to 'char'
           ^~~

Unfortunately the only thing we can do is to hope that the error is
improved in a future version.

GC> Searching online gave me the impression that maybe clang really does
GC> intend to complain about all "uniform initialization"

 No, I don't think so. It compiles code using uniform initialization just
fine and I think it really just objects to converting int to char using
braces here.

 Regards,
VZ

pgpfhw3JuZmYt.pgp
Description: PGP signature