<div dir="ltr">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.<div><br></div><div>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.</div><div><br>Sasha</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 18, 2016 at 7:48 AM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span class="">On Thu, Nov 17, 2016 at 9:52 AM, Richard Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>> On 17 Nov 2016 8:56 am, "Reid Kleckner" <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br></span><span class="">>> In <a href="https://reviews.llvm.org/D24289#598169" target="_blank">https://reviews.llvm.org/<wbr>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></span>[...]<span class=""><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></span>>> type. I don't see how enums are special here. [...]<span class=""><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" target="_blank">http://eel.is/c++draft/dcl.<wbr>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></span><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><span class=""><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></span><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" target="_blank">http://www.open-std.org/jtc1/<wbr>sc22/wg21/docs/cwg_defects.<wbr>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><span class=""><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></span><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>
</blockquote></div><br></div>