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

[faisalv]

Summary

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

[aaron.ballman]

clang/include/clang/Sema/DeclSpec.h

1837

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

[wchilders]

Generally agree with this direction; Are there plans for migrating https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Specifiers.h in a similar fashion, for consistency?

[faisalv]

In D91035#2383167, @wchilders wrote:

Generally agree with this direction; Are there plans for migrating https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Specifiers.h in a similar fashion, for consistency?

yup - i plan to get around to many of these - assuming time cooperates.

Am currently working on Decl::Kind...

clang/include/clang/Sema/DeclSpec.h

1837

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!

[aaron.ballman]

clang/include/clang/Sema/DeclSpec.h

1837

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.

[faisalv]

clang/include/clang/Sema/DeclSpec.h

1837

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!

[aaron.ballman]

clang/include/clang/Sema/DeclSpec.h

1837

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()).

[faisalv]

This revision includes the following changes to the initial patch:

• revert the bit-field to unsigned from enum (so as to avoid that nettlesome gcc warning)
• specified a fixed underlying type of 'unsigned char' for the enum FunctionDefinitionKind
• added static_casts when initiatilizing or assigning to the bit-field (which as Aaron astutely noticed was confined to the ctor and setter)

Thank you!

[rsmith]

clang/include/clang/Sema/DeclSpec.h

1762

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?

1837

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.)

[aaron.ballman]

clang/include/clang/Sema/DeclSpec.h

1762

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.

1837

Thank you for the reminder -- that was the MSVC oddity I was mentioning but couldn't recall the details of!

[wchilders]

This comment was removed by wchilders.

[wchilders]

clang/include/clang/Sema/DeclSpec.h

1762

Lost my original comment... I guess I still don't know how to use Phabricator :(

I see both arguments here, I think I agree more with @rsmith as I generally prefer less "mental indirection"/clarity over less typing.

That said, there's also a potential middle ground here. There is a fair bit of inconsistency in enum naming, looking at Specifiers.h for instance, sometimes "Specifier" is spelled "Specifier" and other times it's spelled "Spec" or "Specifiers" (and actually looking closer, it doesn't appear that TypeSpecifiersPipe is ever used). Perhaps it would be good to standardize the short names, and perhaps use something like FnDefKind or FunctionDefKind -- both of which are notably shortly, but still reasonably understandable and specific names. Just a thought :)

[wchilders]

clang/include/clang/Sema/DeclSpec.h

1762

Correction TypeSpecifiersPipe is used, just a bit strangely -- one value, TSP_pipe is assigned to a bit field as a flag, rather than true.

Also notably shorter* (whoops)

[faisalv]

This diff makes the following changes to the previous patch (based on feedback from Richard, Aaron and Wyatt):

• avoid introducing an initialism (FDK) into the clang namespace and unabbreviated each corresponding use to 'FunctionDefinitionKind'. Let me know if it seems too verbose - if so, perhaps a compromise along Wyatt's suggestion might behoove our source.
• changed the destination type from 'unsigned' to 'unsigned char' in our static_casts.
  • is that preferred, or should i have left it as 'unsigned'?
  • is there any real benefit here to specifying an underlying type of 'unsigned char' for our enum (that is never used as an opaque enum).

Richard, thanks for stepping in and enlightening us as to why the world is still not ready for type-safe enum bit-fields! (spoiler: MSVC's outré ABI choice in layout)

P.S. Also, for those of you like me, who tend to be sloppy with their English (unlike Richard in all his meticulous glory ;) - and were unfamiliar with the nuances behind the term 'initialism' - let me direct you to a lesson from one of the greatest broadcasters of our time: https://youtu.be/FyJsvT3Eo4c

[tschuett]

I started with specifiers.h here: https://reviews.llvm.org/D91409, but it is not yet committed.

[faisalv]

yay!

Thanks Thorsten - if no one else does it - i'll try and commit this for you
later today :)

Faisal Vali

[faisalv]

*ping*

[aaron.ballman]

clang/include/clang/Sema/DeclSpec.h

1756

We don't gain a whole lot by making this unsigned char since we're not storing it anywhere -- leave as the default int and change the static_cast<>s?

clang/lib/Sema/SemaDecl.cpp

9163

Might as well hit these formatting fixes since we're touching the lines anyway.

[faisalv]

committed here: https://github.com/llvm/llvm-project/commit/9930d4dff31a130890f21a64f43d530a83ae3d0a

Thank you Aaron, Richard and Wyat!!

clang/include/clang/Sema/DeclSpec.h

 
 /// Described the kind of function definition (if any) provided for
 /// a function.
-enum FunctionDefinitionKind {
-  FDK_Declaration,
-  FDK_Definition,
-  FDK_Defaulted,
-  FDK_Deleted
+enum class FunctionDefinitionKind : unsigned char {
+  Declaration,
+  Definition,
+  Defaulted,
+  Deleted
 };
 
 enum class DeclaratorContext {

   Declarator(const DeclSpec &ds, DeclaratorContext C)
       : DS(ds), Range(ds.getSourceRange()), Context(C),
         InvalidType(DS.getTypeSpecType() == DeclSpec::TST_error),
-        GroupingParens(false), FunctionDefinition(FDK_Declaration),
+        GroupingParens(false), FunctionDefinition(static_cast<unsigned char>(
+                                   FunctionDefinitionKind::Declaration)),
         Redeclaration(false), Extension(false), ObjCIvar(false),
         ObjCWeakProperty(false), InlineStorageUsed(false),
         Attrs(ds.getAttributePool().getFactory()), AsmLabel(nullptr),

   void setEllipsisLoc(SourceLocation EL) { EllipsisLoc = EL; }
 
   void setFunctionDefinitionKind(FunctionDefinitionKind Val) {
-    FunctionDefinition = Val;
+    FunctionDefinition = static_cast<unsigned char>(Val);
   }
 
   bool isFunctionDefinition() const {
-    return getFunctionDefinitionKind() != FDK_Declaration;
+    return getFunctionDefinitionKind() != FunctionDefinitionKind::Declaration;
   }
 
   FunctionDefinitionKind getFunctionDefinitionKind() const {

clang/lib/Parse/ParseCXXInlineMethods.cpp

   // or if we are about to parse function member template then consume
   // the tokens and store them for parsing at the end of the translation unit.
   if (getLangOpts().DelayedTemplateParsing &&
-      D.getFunctionDefinitionKind() == FDK_Definition &&
+      D.getFunctionDefinitionKind() == FunctionDefinitionKind::Definition &&
       !D.getDeclSpec().hasConstexprSpecifier() &&
       !(FnD && FnD->getAsFunction() &&
         FnD->getAsFunction()->getReturnType()->getContainedAutoType()) &&

clang/lib/Parse/ParseDeclCXX.cpp

     if (getLangOpts().MicrosoftExt && DeclaratorInfo.isDeclarationOfFunction())
       TryConsumePureSpecifier(/*AllowDefinition*/ true);
 
-    FunctionDefinitionKind DefinitionKind = FDK_Declaration;
+    FunctionDefinitionKind DefinitionKind = FunctionDefinitionKind::Declaration;
     // function-definition:
     //
     // In C++11, a non-function declarator followed by an open brace is a
     // braced-init-list for an in-class member initialization, not an
     // erroneous function definition.
     if (Tok.is(tok::l_brace) && !getLangOpts().CPlusPlus11) {
-      DefinitionKind = FDK_Definition;
+      DefinitionKind = FunctionDefinitionKind::Definition;
     } else if (DeclaratorInfo.isFunctionDeclarator()) {
       if (Tok.isOneOf(tok::l_brace, tok::colon, tok::kw_try)) {
-        DefinitionKind = FDK_Definition;
+        DefinitionKind = FunctionDefinitionKind::Definition;
       } else if (Tok.is(tok::equal)) {
         const Token &KW = NextToken();
         if (KW.is(tok::kw_default))
-          DefinitionKind = FDK_Defaulted;
+          DefinitionKind = FunctionDefinitionKind::Defaulted;
         else if (KW.is(tok::kw_delete))
-          DefinitionKind = FDK_Deleted;
+          DefinitionKind = FunctionDefinitionKind::Deleted;
         else if (KW.is(tok::code_completion)) {
           Actions.CodeCompleteAfterFunctionEquals(DeclaratorInfo);
           cutOffParsing();

     // C++11 [dcl.attr.grammar] p4: If an attribute-specifier-seq appertains
     // to a friend declaration, that declaration shall be a definition.
     if (DeclaratorInfo.isFunctionDeclarator() &&
-        DefinitionKind == FDK_Declaration && DS.isFriendSpecified()) {
+        DefinitionKind == FunctionDefinitionKind::Declaration &&
+        DS.isFriendSpecified()) {
       // Diagnose attributes that appear before decl specifier:
       // [[]] friend int foo();
       ProhibitAttributes(FnAttrs);
     }
 
-    if (DefinitionKind != FDK_Declaration) {
+    if (DefinitionKind != FunctionDefinitionKind::Declaration) {
       if (!DeclaratorInfo.isFunctionDeclarator()) {
         Diag(DeclaratorInfo.getIdentifierLoc(), diag::err_func_def_no_params);
         ConsumeBrace();

clang/lib/Parse/ParseExpr.cpp

 
   // Parse the block-declarator.
   Declarator DeclaratorInfo(DS, DeclaratorContext::BlockLiteral);
-  DeclaratorInfo.setFunctionDefinitionKind(FDK_Definition);
+  DeclaratorInfo.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   ParseDeclarator(DeclaratorInfo);
 
   MaybeParseGNUAttributes(DeclaratorInfo);

   // Parse the return type if present.
   DeclSpec DS(AttrFactory);
   Declarator ParamInfo(DS, DeclaratorContext::BlockLiteral);
-  ParamInfo.setFunctionDefinitionKind(FDK_Definition);
+  ParamInfo.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   // FIXME: Since the return type isn't actually parsed, it can't be used to
   // fill ParamInfo with an initial valid range, so do it manually.
   ParamInfo.SetSourceRange(SourceRange(Tok.getLocation(), Tok.getLocation()));

clang/lib/Parse/Parser.cpp

                                    Scope::CompoundStmtScope);
     Scope *ParentScope = getCurScope()->getParent();
 
-    D.setFunctionDefinitionKind(FDK_Definition);
+    D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
     Decl *DP = Actions.HandleDeclarator(ParentScope, D,
                                         TemplateParameterLists);
     D.complete(DP);

                                    Scope::CompoundStmtScope);
     Scope *ParentScope = getCurScope()->getParent();
 
-    D.setFunctionDefinitionKind(FDK_Definition);
+    D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
     Decl *FuncDecl = Actions.HandleDeclarator(ParentScope, D,
                                               MultiTemplateParamsArg());
     D.complete(FuncDecl);

clang/lib/Sema/SemaDecl.cpp

 }
 
 Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
-  D.setFunctionDefinitionKind(FDK_Declaration);
+  D.setFunctionDefinitionKind(FunctionDefinitionKind::Declaration);
   Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());
 
   if (OriginalLexicalContext && OriginalLexicalContext->isObjCContainer() &&

     // If a function is defined as defaulted or deleted, mark it as such now.
     // We'll do the relevant checks on defaulted / deleted functions later.
     switch (D.getFunctionDefinitionKind()) {
-      case FDK_Declaration:
-      case FDK_Definition:
+      case FunctionDefinitionKind::Declaration:
+      case FunctionDefinitionKind::Definition:
         break;
 
-      case FDK_Defaulted:
+      case FunctionDefinitionKind::Defaulted:
         NewFD->setDefaulted();
         break;
 
-      case FDK_Deleted:
+      case FunctionDefinitionKind::Deleted:
         NewFD->setDeletedAsWritten();
         break;
     }

   // because Sema::ActOnStartOfFunctionDef has not been called yet.
   if (const auto *NBA = NewFD->getAttr<NoBuiltinAttr>())
     switch (D.getFunctionDefinitionKind()) {
-    case FDK_Defaulted:
-    case FDK_Deleted:
+    case FunctionDefinitionKind::Defaulted:
+    case FunctionDefinitionKind::Deleted:
       Diag(NBA->getLocation(),
            diag::err_attribute_no_builtin_on_defaulted_deleted_function)
           << NBA->getSpelling();
       break;
-    case FDK_Declaration:
+    case FunctionDefinitionKind::Declaration:
       Diag(NBA->getLocation(), diag::err_attribute_no_builtin_on_non_definition)
           << NBA->getSpelling();
       break;
-    case FDK_Definition:
+    case FunctionDefinitionKind::Definition:
       break;
     }
 

     ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
         ParentScope, D, TemplateParameterLists, Bases);
 
-  D.setFunctionDefinitionKind(FDK_Definition);
+  D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
   Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
 

clang/lib/Sema/SemaOpenMP.cpp

       llvm::omp::TraitProperty::implementation_extension_disable_implicit_base);
   // If no base was found we create a declaration that we use as base.
   if (Bases.empty() && UseImplicitBase) {
-    D.setFunctionDefinitionKind(FDK_Declaration);
+    D.setFunctionDefinitionKind(FunctionDefinitionKind::Declaration);
     Decl *BaseD = HandleDeclarator(S, D, TemplateParamLists);
     BaseD->setImplicit(true);
     if (auto *BaseTemplD = dyn_cast<FunctionTemplateDecl>(BaseD))

clang/lib/Sema/SemaType.cpp

   // Only warn if this declarator is declaring a function at block scope, and
   // doesn't have a storage class (such as 'extern') specified.
   if (!D.isFunctionDeclarator() ||
-      D.getFunctionDefinitionKind() != FDK_Declaration ||
+      D.getFunctionDefinitionKind() != FunctionDefinitionKind::Declaration ||
       !S.CurContext->isFunctionOrMethod() ||
-      D.getDeclSpec().getStorageClassSpec()
-        != DeclSpec::SCS_unspecified)
+      D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_unspecified)
     return;
 
   // Inside a condition, a direct initializer is not permitted. We allow one to

           !(S.getLangOpts().CPlusPlus &&
             (T->isDependentType() || T->isRecordType()))) {
         if (T->isVoidType() && !S.getLangOpts().CPlusPlus &&
-            D.getFunctionDefinitionKind() == FDK_Definition) {
+            D.getFunctionDefinitionKind() ==
+                FunctionDefinitionKind::Definition) {
           // [6.9.1/3] qualified void return is invalid on a C
           // function definition.  Apparently ok on declarations and
           // in C++ though (!)

   //   The empty list in a function declarator that is not part of a definition
   //   of that function specifies that no information about the number or types
   //   of the parameters is supplied.
-  if (!LangOpts.CPlusPlus && D.getFunctionDefinitionKind() == FDK_Declaration) {
+  if (!LangOpts.CPlusPlus &&
+      D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration) {
     bool IsBlock = false;
     for (const DeclaratorChunk &DeclType : D.type_objects()) {
       switch (DeclType.Kind) {