Although I agree with your philosophical discussion and suggestions, the reality is that MSVC's behavior is not a bug and compilers are free to interpret enum bitfields with no explicit underlying type in any way they want (see spec reference in GCC bug link), with a signed interpretation being a valid one. I'd say it's undefined behavior in C/C++ to store an enum in a bitfield without specifying an underlying type, since the compiler is free to interpret this bitfield in any way it wants -- in general, if you haven't specified an underlying type you should probably be warned when trying to store it in a bitfield because the compiler may not do what you expect.
With that said, I'm happy to put this warning behind a flag, or remove it altogether from Clang. This is an important feature for Blink and whether or not it lands, we will be using it for compiling our own code. I submitted the patch upstream to Clang since I thought all developers should be aware of this undefined behavior. However, if you feel that developers who write code that uses enum bitfields should be free to write non-MSVC-compatible code, then there's no need for this to be in the main Clang release. Sasha On Fri, Nov 18, 2016 at 7:48 AM, Arthur O'Dwyer <arthur.j.odw...@gmail.com> wrote: > On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > On 17 Nov 2016 8:56 am, "Reid Kleckner" <r...@google.com> wrote: > >> In https://reviews.llvm.org/D24289#598169, @rsmith wrote: > >>> This is causing warnings to fire for headers shared between C and C++, > >>> where the "give the enum an unsigned underlying type" advice doesn't > work, > >>> and where the code in question will never be built for the MS ABI. It > seems > >>> really hard to justify this being on by default. > >>> > >>> I'm going to turn it off by default for now, but we should probably > >>> consider turning it back on by default when targeting the MS ABI (as a > "your > >>> code is wrong" warning rather than a "your code is not portable" > warning). > [...] > >>> Yeah, suggesting adding an underlying type to the enum to solve this > >>> problem seems like a bad idea, since that fundamentally changes the > nature > >>> of the enum -- typically allowing it to store a lot more values, and > making > >>> putting it in a bitfield a bad idea. > > > >> Any time you use a bitfield it stores fewer values than the original > integer > >> type. I don't see how enums are special here. [...] > > > > The range of representable values for a bitfield with no fixed underlying > > type is actually smaller than that of its underlying type. See > > http://eel.is/c++draft/dcl.enum#8 > > > > So a bitfield of width equal to the number of bits needed to store any > > enumerator does not have fewer values than the original type. > > My understanding (from osmosis and practice more than from reading the > standard) is that programmers are more likely to specify an "unnaturally > narrow" underlying type (e.g. "int8_t") than to specify an "unnaturally > wide" underlying type (e.g. "int32_t". When I specify an underlying type, > I'm saying "The compiler is going to do the wrong thing with this type's > *storage* by default"; I'm not saying anything about the type's *value > range*. > The same goes for bit-fields: I specify a number of bits after the colon > because the compiler would otherwise do the wrong thing with *storage*, > not because I'm trying to change the semantics of the *values* involved. > > > >> Do you have any better suggestions for people that want this code to do > the > >> right thing when built with MSVC? > >> > >> enum E /* : unsigned */ { E0, E1, E2, E3 }; > >> struct A { E b : 2; }; > >> int main() { > >> A a; > >> a.b = E3; > >> return a.b; // comes out as -1 without the underlying type > >> } > >> > >> Widening the bitfield wastes a bit. Making the bitfield a plain integer > and > >> cast in and out of the enum type, but that obscures the true type of the > >> bitfield. So, I still support this suggestion. > > The safest fix is to just change ": 2" to ": 3", even though that "wastes > a bit" (really it wastes 0 bits in most cases and 32 to 64 bits in some > cases). > > If I had my druthers, the compiler would be completely silent unless it > detected exactly the cases that would result in changes to the semantics of > enum *values*. That is, > > when declaring a bit-field of enum type E with width B bits: > if E has an enumerator e whose value requires >B bits: > warn that e cannot be stored in the bit-field > if a fixit is really required, suggest increasing the bit-field's > width > if E has an enumerator e whose positive value requires exactly B > bits, and E's underlying type is signed: > warn that e cannot be stored in the bit-field when MSVC semantics > are in use > in C++, append the note that this happens because E's underlying > type is signed > if a fixit is really required, suggest increasing the bit-field's > width > > Changing the width of the bit-field can affect only the layout of the > struct in question; you could even static_assert that the size *doesn't* > change, if there are padding bits available. > Changing a whole type from signed to unsigned seems like it would have > more dangerous action-at-a-distance than just increasing the width of the > bit-field: that would *unavoidably* affect comparisons (and overload > resolutions?) across the entire codebase using that type. > http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#172 > Again I'm making a philosophical distinction between the details of > storage (e.g. struct layout) and the actual semantics of values and types > (e.g. signedness). > > The problem is with the bit-field inside the struct, so IMHO the solution > should involve changing the bit-field and/or the struct. Not altering the > enum type... which by the way, the programmer might not even control, > right? What if the relevant enum type comes from a third-party library? > > > How about we consider msvc's behaviour to just be a bug, and zero-extend > > loads from enum bitfields with no negative enumerators? Since loading > such a > > negative value results in UB, this wouldn't change the meaning of any > > program with defined behaviour, and still respects the MS ABI. > > SGTM, at least to put this behavior under a separately toggleable > command-line switch. (-fms-enum-bitfields?) > > my $.02, > –Arthur >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
