aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Sema/DeclSpec.h:1762 }; +using FDK = FunctionDefinitionKind; ---------------- rsmith wrote: > 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? That will change uses like this: ``` D.setFunctionDefinitionKind(FDK::Definition); ``` into ``` D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition); ``` which repeats the enumeration name twice (once for the function and once for the enumerator) and is rather unfortunate. I'm not certain it's more unfortunate than putting an initialism into the `clang` namespace though. ================ Comment at: clang/include/clang/Sema/DeclSpec.h:1837 /// Actually a FunctionDefinitionKind. - unsigned FunctionDefinition : 2; + FunctionDefinitionKind FunctionDefinition : 2; ---------------- rsmith wrote: > 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.) Thank you for the reminder -- that was the MSVC oddity I was mentioning but couldn't recall the details of! 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
