<div dir="ltr">On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>> On 17 Nov 2016 8:56 am, "Reid Kleckner" <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>>> In <a href="https://reviews.llvm.org/D24289#598169">https://reviews.llvm.org/D24289#598169</a>, @rsmith wrote:<br>>>> This is causing warnings to fire for headers shared between C and C++,<br>>>> where the "give the enum an unsigned underlying type" advice doesn't work,<br>>>> and where the code in question will never be built for the MS ABI. It seems<br>>>> really hard to justify this being on by default.<br>>>><br>>>> I'm going to turn it off by default for now, but we should probably<br>>>> consider turning it back on by default when targeting the MS ABI (as a "your<br>>>> code is wrong" warning rather than a "your code is not portable" warning).<br>[...]<br>>>> Yeah, suggesting adding an underlying type to the enum to solve this<br>>>> problem seems like a bad idea, since that fundamentally changes the nature<br>>>> of the enum -- typically allowing it to store a lot more values, and making<br>>>> putting it in a bitfield a bad idea.<br>><br>>> Any time you use a bitfield it stores fewer values than the original integer<br>>> type. I don't see how enums are special here. [...]<br>><br>> The range of representable values for a bitfield with no fixed underlying<br>> type is actually smaller than that of its underlying type. See<br>> <a href="http://eel.is/c++draft/dcl.enum#8">http://eel.is/c++draft/dcl.enum#8</a><br>><br>> So a bitfield of width equal to the number of bits needed to store any<br>> enumerator does not have fewer values than the original type.<div><br></div><div>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 <i>storage</i> by default"; I'm not saying anything about the type's <i>value range</i>.</div><div>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 <i>storage</i>, not because I'm trying to change the semantics of the <i>values</i> involved.</div><div><br></div><div><br></div><div>>> Do you have any better suggestions for people that want this code to do the<br>>> right thing when built with MSVC?<br>>><br>>>   enum E /* : unsigned */ { E0, E1, E2, E3 };<br>>>   struct A { E b : 2; };<br>>>   int main() {<br>>>     A a;<br>>>     a.b = E3;<br>>>     return a.b; // comes out as -1 without the underlying type<br>>>   }<br>>><br>>> Widening the bitfield wastes a bit. Making the bitfield a plain integer and<br>>> cast in and out of the enum type, but that obscures the true type of the<br>>> bitfield. So, I still support this suggestion.</div><div><br></div><div>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).</div><div><br></div><div>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 <i>values</i>. That is,</div><div><br></div><div>    when declaring a bit-field of enum type E with width B bits:</div><div>      if E has an enumerator e whose value requires >B bits:</div><div>        warn that e cannot be stored in the bit-field</div><div>        if a fixit is really required, suggest increasing the bit-field's width</div><div>      if E has an enumerator e whose positive value requires exactly B bits, and E's underlying type is signed:</div><div>        warn that e cannot be stored in the bit-field when MSVC semantics are in use</div><div>        in C++, append the note that this happens because E's underlying type is signed</div><div><div>        if a fixit is really required, suggest increasing the bit-field's width</div></div><div><br></div><div>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.</div><div>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 <i>unavoidably</i> affect comparisons (and overload resolutions?) across the entire codebase using that type.<br></div><div><div><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#172">http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#172</a><br></div></div><div>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).</div><div><br></div><div>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?</div><div><br></div><div>> How about we consider msvc's behaviour to just be a bug, and zero-extend<br>> loads from enum bitfields with no negative enumerators? Since loading such a<br>> negative value results in UB, this wouldn't change the meaning of any<br>> program with defined behaviour, and still respects the MS ABI.</div><div><br></div><div>SGTM, at least to put this behavior under a separately toggleable command-line switch. (-fms-enum-bitfields?)</div><div><br></div><div>my $.02,</div><div>–Arthur</div></div>