Re: [lmi] lmi tests and MSVC (was: Don't initialize constexpr variable with std::ldexp())

[Greg Chicares]

On 2/19/23 00:20, Vadim Zeitlin wrote:
[...]
> On Mon, 24 Apr 2017 22:19:46 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> It would be good to know whether at least lmi's unit tests (except for
> GC> 'any_member_test') build and report zero errors for msvc. I'd especially
> GC> like to know whether the very recent 'bourn_cast_test' does.
> 
>  I still don't have a build system allowing me to build all the lmi tests
> with MSVC, but I tried at least this one (even though it's not so recent
> any more) and it fails with the following error:
> 
> ???? uncaught exception: std::runtime_error: Cast would transgress upper 
> limit.
> 
> This happens in test_same<long long int> on the line
> 
>       T imax = bourn_cast<T>(max);
> 
> because "limit" and "from" are both equal to 9.2233720368547758e+18 (or
> 43e0000000000000 as IEEE-754 binary, i.e. 1b63) inside bourn_cast(). I
> don't really understand why should we throw in case of equality nor why are
> they equal only with MSVC but not the other compilers. I could look at this
> in more details but I wanted to ask first just in case the problem is
> already immediately clear to you?

Here, 'max' is of type 'long double'; test_same() says:

    // an 80-bit long double type whose 64-bit mantissa suffices to
    // test the limits of every integral type up to 64 digits exactly
    // because it can distinguish +/-(2^64) from +/-(2^64 - 1).

To guard against compilers that lack the necessary support,
we could do something like this:

+    // [comment explaining the extra condition]
-    if(traits::is_integer)
+    if(traits::is_integer && 64 <= LDBL_MANT_DIG)

I'll commit such a revision next week unless you disagree.

>  FWIW there are also several compilation warnings in this test with MSVC:
> 
> bourn_cast_test.cpp(473,36): warning C4305: 'initializing': truncation from 
> 'const unsigned __int64' to 'float'
> bourn_cast_test.cpp(90,9): warning C4805: '==': unsafe mix of type 'bool' and 
> type 'const long double' in operation
> bourn_cast_test.cpp(737): message : see reference to function template 
> instantiation 'void test_same<bool>(const char *,int)' being compiled
> bourn_cast_test.cpp(89,41): warning C4805: '==': unsafe mix of type 'bool' 
> and type 'long double' in operation
> bourn_cast_test.cpp(166,1): warning C4245: 'initializing': conversion from 
> 'int' to 'CFrom', signed/unsigned mismatch
> bourn_cast_test.cpp(780): message : see reference to function template 
> instantiation 'void test_signednesses<true,false>(const char *,int)' being 
> compiled
> bourn_cast_test.cpp(167,1): warning C4245: 'initializing': conversion from 
> 'int' to 'IFrom', signed/unsigned mismatch
> bourn_cast_test.cpp(168,1): warning C4245: 'initializing': conversion from 
> '__int64' to 'LFrom', signed/unsigned mismatch
> bourn_cast_test.cpp(170,1): warning C4245: 'initializing': conversion from 
> 'int' to 'CTo', signed/unsigned mismatch
> bourn_cast_test.cpp(784): message : see reference to function template 
> instantiation 'void test_signednesses<false,true>(const char *,int)' being 
> compiled
> bourn_cast_test.cpp(171,1): warning C4245: 'initializing': conversion from 
> 'int' to 'ITo', signed/unsigned mismatch
> bourn_cast_test.cpp(172,1): warning C4245: 'initializing': conversion from 
> '__int64' to 'LTo', signed/unsigned mismatch
> 
> but they look mostly harmless (even though definitely annoying).

They would occur for other compilers as well, but are suppressed
by pragmata. I'd accept a patch adding msvc pragmata.

> GC> BTW, does LMI_MSVCRT serve any purpose any more? It's used only in
> GC> 'numeric_io*', to work around printf() problems in the msw system C RTL
> GC> like missing support for "%Lf" and formatting infinity as "1.#INF". The
> GC> mingw* toolchains work around that with libmingwex. Does msvc now use an
> GC> updated C RTL that doesn't have these defects, so that these LMI_MSVCRT
> GC> workarounds can be expunged?
> 
>  I wanted to check this one as well, but numeric_io_test.cpp currently
> fails even with LMI_MSVCRT defined:
> 
> ???? test failed:   '15' == '12'
> [file numeric_io_test.cpp, line 141]
> 
> ???? test failed:   '15' == '16'
> [file numeric_io_test.cpp, line 174]
> Conversions:
>   2/3, lmi  : 1.032e-05 s mean;          9 us least of 970 runs
>   inf, lmi  : 5.285e-06 s mean;          4 us least of 1893 runs
> 
> ???? 2 test errors detected; 441 tests succeeded
> 
> Again, it doesn't seem immediately obvious to me what is wrong here and I
> could try to find out if you think this would be useful.

Commit e90e53748f697ea probably explains msvc's failure on line 174
as well as valgrind's. I would guess that the failure on line 141 has
a similar cause.

BTW, given that we use clang's sanitizers, do we still care about
valgrind?

>  But, just to check, I've tried removing LMI_MSVCRT definition from
> config.hpp and it seems like it's still useful because without it I also
> get
> 
> ???? uncaught exception: std::invalid_argument: Attempt to convert string 
> 'i.nf' from type class std::basic_string<char,struct 
> std::char_traits<char>,class std::allocator<char> > to type double found 
> nothing valid to convert.
> 
> towards the end. As you can see, the result of sprintf("%#.*f", 0, inf)
> with MSVC is "i.nf" which seems weird but OTOH I have no idea what is the
> output supposed to be when the "#" flag is used with infinity and precision

In that case, C99 says the output must contain a decimal point.
If the msvc runtime converts a numeric value to a string that it
refuses to convert back to a numeric value, than I'd say that's
a QoI issue.

> I could look at this further and maybe propose a patch if you think it's
> worth doing this -- just please let me know.

I don't think this one merits any further attention. I think
bourn_cast<>() has general value because it's a correct
implementation of something that's hard to do correctly.
OTOH, the motivation for the numeric_io stuff was to do
something that stream inserters and extractors already do
correctly, but with much greater speed; C++20's std::format
probably supersedes it. These unit tests may still be useful
for validating std::format, but if ms considers "i.nf"
acceptable, then that's not a defect of the lmi test suite.