Re: [lmi] PATCH: MSVC compilation fix

[Vadim Zeitlin]

On Fri, 3 Jun 2022 01:13:49 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 6/2/22 15:11, Vadim Zeitlin wrote:
GC> > 
GC> >  I'd like to submit a small patch, which is basically this:
GC> [...]
GC> >  PR 212 (https://github.com/let-me-illustrate/lmi/pull/212).
GC> 
GC> Committed.

 Thanks!

GC> Not yet pushed, because I recently pushed something
GC> without running ./nychthemeral_test beforehand, and it broke
GC> something--so I'll wait until I run the complete test suite.

 I'm relatively confident this change shouldn't break anything.

GC> >  Finally, to finish with MSVC-related topics, there are a couple of new
GC> > warnings in the recently added code:
GC> > 
GC> > fdlibm_log1p.c(192): warning C4701: potentially uninitialized local 
variable 'f' used
GC> > fdlibm_log1p.c(193): warning C4701: potentially uninitialized local 
variable 'hu' used
GC> > fdlibm_log1p.c(196): warning C4701: potentially uninitialized local 
variable 'c' used
GC> > fdlibm_expm1.c(234): warning C4701: potentially uninitialized local 
variable 'c' used
GC> > 
GC> > I don't think any of them indicates any actual problems, but I wonder if 
it
GC> > might still make sense to initialize these variables just to avoid even
GC> > having to think about it.
GC> 
GC> Agreed. Is there even a syntax to do that all on one line? Ah, of course:
GC>   :'<,'>s/[,;]/=0&/g
GC> That passes the unit tests. Now let's see if any compiler complains about
GC> useless initializations, like:
GC> 
GC>     int32_t k=0,hx=0,hu=0,ax=0;
GC> [...'hx' is immediately initialized to a different value...]
GC>     hx = hi_int(x);                             // high word of x

 I think some of them might, yes... but I'm not totally sure, I haven't
seen such warnings since quite some time, so I wonder if something has
changed and if, perhaps, the compiler developers decided they were not
useful, after all.

GC> I've been resisting the temptation to rewrite this code, but this ancient
GC> style is not good in this century. I also thought I might try compiling it
GC> as C++, because I recently read somewhere that that might make it faster;

 I haven't looked at this code very carefully, so I could be missing
something, but there doesn't seem to be anything in it that could possibly
benefit from using C++.

GC> but I hesitate to do so, because AFAICT type punning with unions is legal
GC> in C, but UB in C++.

 Formally speaking it is, of course, and you're supposed to be using
memcpy() instead, but I doubt very much that any compiler dares to really
break this, considering how common it is. OTOH modern compilers did already
surprise me many times in the past, so why not again.

GC> > (there are, BTW, still plenty of other warnings in MSVC builds related to
GC> > using enums and doubles in arithmetic expressions, that I still think 
ought
GC> > to be really fixed rather than suppressed, but I had already posted about
GC> > them
GC> 
GC> Is that your recent message about fixing some UB, which I have
GC> put off reading in order to fix negation of INT_MAX? If not, could
GC> you remind me where to find it, please?

 No, sorry, this is completely different and is much older. I think the
first time I mentioned this issue was in this post:

        https://lists.nongnu.org/archive/html/lmi/2021-04/msg00029.html

where I wrote

VZ>  However the other ones are more problematic. I do agree with clang that
VZ> converting mcenum_mode to double or comparing oenum_premium_flexibility
VZ> with double is iffy, but I'm not sure if you'd consider doing anything
VZ> about this. Personally I think we should add some function converting
VZ> double to enum and throwing if the double value doesn't correspond to any
VZ> of the enum values, but this won't be a trivial change as it would
VZ> introduce a possibility of throwing an exception from places where it
VZ> couldn't happen before. The minimum change sufficient to fix this warning
VZ> that I see is to convert enum values to double, possibly via an explicitly
VZ> named function rather than a cast, but this seems ugly and I'm still not
VZ> sure if you'd be willing to do it.

I think I mentioned it a couple of times after this, but I can't find
anything in the list archives somehow... To summarize the issue quickly,
MSVC has its own equivalent of -Wdeprecated-enum-float-conversion and
(clang-specific) -Wdeprecated-anon-enum-enum-conversion and while I could
disable these warnings for it too, just as we do for gcc and clang, I delay
doing it hoping that maybe we can fix them instead.

 If you'd like to check these warnings yourself, removing
-Wno-deprecated-enum-float-conversion from
gcc_version_specific_cxx_warnings in workhorse.make should show most of
them, even if I think clang and MSVC give a few more than gcc does.
Otherwise I can post a full MSVC build log, but, basically, any operation
between enum and a different type (including different enums or floating
point types) is deprecated in C++20 and provokes a warning.

 Please let me know if you'd like more details about this,
VZ

pgpkS9YiXROgl.pgp
Description: PGP signature