Re: [lmi] Fixing build with clang 7 and 64-bit Linux

[Vadim Zeitlin]

On Mon, 12 Nov 2018 18:06:14 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-11-11 19:52, Vadim Zeitlin wrote:
GC> [...]
GC> >   https://github.com/vadz/lmi/pull/103
GC> 
GC> 
https://github.com/vadz/lmi/pull/103/commits/07fba915f6d250dfab5ee06bc79fe88ac75efeef
GC> | facets.cpp:118:50: error: constant expression evaluates to -8193 which
GC> | cannot be narrowed to type 'std::ctype_base::mask' (aka 'unsigned
GC> | short') [-Wc++11-narrowing]
GC> |
GC> | when using clang by simply removing the braces, which impose C++11
GC> | initialization and forbidding narrowing, and allowing the implicit cast
GC> | to unsigned short to take place.
GC> ...
GC> |             // See "Implementation note" above.
GC> | -           constexpr std::ctype_base::mask z = {~std::ctype_base::space};
GC> | +           constexpr std::ctype_base::mask z = ~std::ctype_base::space;
GC> 
GC> See the cited "Implementation note". I think that one or more of
GC>   {gcc; clang; the C++11 and C++17 standards}
GC> is incorrect.

 Sorry, I didn't mention it because I thought it was implicitly clear, but
my assumption was, and still is, that this is a gcc defect because a
variable of unsigned type just can't be brace-initialized with a negative
value, which ~std::ctype_base::space clearly is (at least under Linux).

 Weirdly enough, the following program does give the expected error:
---------------------------------- >8 --------------------------------------
% cat ctype_base_val.cpp
#include <locale>

int main()
{
    constexpr std::ctype_base::mask z = {~std::ctype_base::space};
    return z;
}
% g++-8 -Wall -std=c++17 ctype_base_val.cpp
ctype_base_val.cpp: In function ‘int main()’:
ctype_base_val.cpp:5:65: error: narrowing conversion of ‘-8193’ from ‘int’ to 
‘std::ctype_base::mask’ {aka ‘short unsigned int’} inside { } [-Wnarrowing]
     constexpr std::ctype_base::mask z = {~std::ctype_base::space};
                                                                 ^
---------------------------------- >8 --------------------------------------
but somehow it doesn't happen when the same code is inside a template. I
think I should open a gcc bug report about this, unless you see something
that I'm missing here.


GC> Does clang compile Dietmar Kuehl's original
GC>   rc[C] &= ~std::ctype_base::space;
GC> without warning?

 Yes, even with -Wconversion (which is not used for clang build currently,
but arguably should). But this seems more like something wrong with clang
-Wconversion than anything else because it doesn't give any warnings even
in the code where the value of RHS can't be known. I.e. g++ warning is, in
principle, useful, but it's pretty weird that it says that conversion may
change value when it knows perfectly well that it doesn't (because the
value is known at compile-time).

GC> and if not, then should it?

 I think so, yes.

GC> doesn't that mean that Kuehl's original
GC>   rc[C] &= ~std::ctype_base::space;
GC> is strictly conforming? If so, then maybe only gcc is wrong, and
GC> only gcc needs any workaround (which can later be removed when
GC> they correct the problem), but for clang we can just revert to
GC> the original because it has always been correct.

 Yes, this would also be a possibility.

GC> But is clang also wrong, perhaps in a different way than gcc?
GC> It complains about line 118:
GC>     constexpr std::ctype_base::mask z = {~std::ctype_base::space};
GC>     "constant expression evaluates to -8193"
GC> but the standard says:
GC> 
GC> |  constexpr bitmask operator~(bitmask X){
GC> |  return static_cast<bitmask>(~static_cast<int_type>(X));
GC> |  }
GC> 
GC> so is clang saying that 'static_cast<bitmask>' fails to cast its
GC> value to type <bitmask>? Or did they omit that static_cast in
GC> their standard library implementation?

 I don't think the standard mandates implementing bitmask in this way, i.e.
by defining any overloaded operators for it. The implementation using a
simple "typedef unsigned short mask", which is used by libstdc++, is
conforming too AFAIK and in this case the RHS of the statement "rc[C] &=
~std::ctype_base::space;" has type "int" and not "unsigned short".

 To summarize:

1. I'm pretty sure that the lack of warning for the code in the current
   master is a bug in gcc.
2. I strongly suspect that there is nothing wrong with the original code,
   but that it just interacts unexpectedly badly with gcc -Wconversion
   implementation.
3. And that this implementation definitely seems perfectible, but then so
   does clang's one, although for a diametrically opposite reason.

 And to conclude, I still believe that my patch is the best way to avoid
the warnings with both gcc and clang right now and so should be applied.
Although I definitely should have modified the comment as well.

 What do you think?
VZ