rsmith added inline comments.
================ Comment at: clang/include/clang/Sema/DeclSpec.h:1762 }; +using FDK = FunctionDefinitionKind; ---------------- I don't think it's OK to have an initialism like this in the `clang` namespace scope -- generally-speaking, the larger the scope of a name, the longer and more descriptive the name needs to be. Is spelling out the full name of the enumeration everywhere it appears unacceptably verbose? ================ 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: > > > 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()`). We should be very wary of having bit-fields of enumeration type anyway, because the MS ABI layout rule for bit-fields doesn't pack adjacent bit-fields together if they don't have the same storage type. (That's why we use `unsigned : 1` bit-fields for flags most of the time -- so they'll pack with adjacent `unsigned : 2` bitfields under the MS ABI.) 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
