[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

Wed Nov 11 05:26:05 PST 2020

faisalv added inline comments.

================
Comment at: clang/include/clang/Sema/DeclSpec.h:1837
   /// Actually a FunctionDefinitionKind.
-  unsigned FunctionDefinition : 2;
+  FunctionDefinitionKind FunctionDefinition : 2;

----------------
aaron.ballman wrote:
> faisalv wrote:
> > aaron.ballman wrote:
> > > I think we need to keep this as `unsigned` because some compilers struggle with bit-fields of enumeration types (even when the enumeration underlying type is fixed): https://godbolt.org/z/P8x8Kz
> > As Barry had reminded me - this warning was deemed a bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242.  Are you sure we should still tailor our code to appease it? Is there a config file we can use to #define an ENUM_UNSIGNED_BITFIELD(x) or some such - that does the right thing for most compilers - (and are we even comfortable from a style-guide perpective, with such a conditional-define strategy?
> > 
> > Your thoughts?
> > 
> > Thanks!
> The warning in GCC was a bug, but the fact that GCC issues the warning means `-Werror` builds will not be able to handle it. GCC 9.2 is really recent, so we can't just bump the supported version of GCC to 9.3 to avoid the issue. We could use macros to work around it for GCC, but IIRC, MSVC also had some hiccups over the years with using an enumeration as a bit-field member (I seem to recall it not wanting to pack the bits with surrounding fields, but I could be remembering incorrectly). I'm not certain whether macros are worth the effort, but my personal inclination is to just stick with `unsigned` unless there's a really big win from coming up with something more complex.
Well - the biggest downside of making it unsigned (vs leaving it as an enum) is that each assignment or initialization now requires a static_cast.  

What would you folks suggest:
1) do not modernize such enums into scoped enums
2) scope these enums - sticking to unsigned bit-fields - and add static_casts
3) teach the bots to ignore that gcc warning? (is this even an option)

Thank you!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91035/new/

https://reviews.llvm.org/D91035