aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Sema/DeclSpec.h:1837 /// Actually a FunctionDefinitionKind. - unsigned FunctionDefinition : 2; + FunctionDefinitionKind FunctionDefinition : 2; ---------------- faisalv wrote: > 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! For #2, do you have an idea of how often we'd need to insert the static_casts for this particular enum? I don't think we assign to this field all that often in a place where we only have an integer rather than an enumeration value, so my preference is for #2 on a case-by-case basis (for instance, we could add a helper function to set unsigned bit-fields to an enum value -- we already have one here with `setFunctionDefinitionKind()`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91035/new/ https://reviews.llvm.org/D91035 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
