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

[Faisal Vali via Phabricator via cfe-commits]

faisalv closed this revision.
faisalv added a comment.

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

Thank you Aaron, Richard and Wyat!!


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1756
 /// a function.
-enum FunctionDefinitionKind {
-  FDK_Declaration,
-  FDK_Definition,
-  FDK_Defaulted,
-  FDK_Deleted
+enum class FunctionDefinitionKind : unsigned char {
+  Declaration,

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?



Comment at: clang/lib/Sema/SemaDecl.cpp:9163
 switch (D.getFunctionDefinitionKind()) {
-  case FDK_Declaration:
-  case FDK_Definition:
+  case FunctionDefinitionKind::Declaration:
+  case FunctionDefinitionKind::Definition:

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


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

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv added a comment.

*ping*


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

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

[Faisal Vali via cfe-commits]

yay!

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

Faisal Vali



On Sat, Nov 14, 2020 at 11:08 AM Thorsten via Phabricator <
revi...@reviews.llvm.org> wrote:

> tschuett added a comment.
>
> I started with specifiers.h here: https://reviews.llvm.org/D91409, but it
> is not yet committed.
>
>
> 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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

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


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

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv updated this revision to Diff 305317.
faisalv added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaType.cpp

Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3577,10 +3577,9 @@
   // 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
@@ -5034,7 +5033,8 @@
   !(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 (!)
@@ -5405,7 +5405,8 @@
   //   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) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5939,7 +5939,7 @@
   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(BaseD))
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -5543,7 +5543,7 @@
 }
 
 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() &&
@@ -9160,15 +9160,15 @@
 // 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:
+

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

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1762
 };
+using FDK = FunctionDefinitionKind;
 

wchilders wrote:
> aaron.ballman wrote:
> > 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.
> 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 :)
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)


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

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

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1762
 };
+using FDK = FunctionDefinitionKind;
 

aaron.ballman wrote:
> 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.
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 :)


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

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

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added a comment.

Replying to inline comments.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv updated this revision to Diff 305020.
faisalv added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaType.cpp

Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3577,7 +3577,7 @@
   // 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() != FDK::Declaration ||
   !S.CurContext->isFunctionOrMethod() ||
   D.getDeclSpec().getStorageClassSpec()
 != DeclSpec::SCS_unspecified)
@@ -5034,7 +5034,7 @@
   !(S.getLangOpts().CPlusPlus &&
 (T->isDependentType() || T->isRecordType( {
 if (T->isVoidType() && !S.getLangOpts().CPlusPlus &&
-D.getFunctionDefinitionKind() == FDK_Definition) {
+D.getFunctionDefinitionKind() == FDK::Definition) {
   // [6.9.1/3] qualified void return is invalid on a C
   // function definition.  Apparently ok on declarations and
   // in C++ though (!)
@@ -5405,7 +5405,8 @@
   //   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() == FDK::Declaration) {
 bool IsBlock = false;
 for (const DeclaratorChunk &DeclType : D.type_objects()) {
   switch (DeclType.Kind) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5939,7 +5939,7 @@
   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(FDK::Declaration);
 Decl *BaseD = HandleDeclarator(S, D, TemplateParamLists);
 BaseD->setImplicit(true);
 if (auto *BaseTemplD = dyn_cast(BaseD))
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -5543,7 +5543,7 @@
 }
 
 Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
-  D.setFunctionDefinitionKind(FDK_Declaration);
+  D.setFunctionDefinitionKind(FDK::Declaration);
   Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());
 
   if (OriginalLexicalContext && OriginalLexicalContext->isObjCContainer() &&
@@ -9160,15 +9160,15 @@
 // 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 FDK::Declaration:
+  case FDK::Definition:
 break;
 
-  case FDK_Defaulted:
+  case FDK::Defaulted:
 NewFD->setDefaulted();
 break;
 
-  case FDK_Deleted:
+  case FDK::Deleted:
 NewFD->setDeletedAsWritten();
 break;
 }
@@ -9871,17 +9871,17 @@
   // because Sema::ActOnStartOfFunctionDef has not been called yet.
   if (const auto *NBA = NewFD->getAttr())
 switch (D.getFunctionDefinitionKind()) {
-case FDK_Defaulted:
-case FDK_Deleted:
+case FDK::Defaulted:
+case FDK::Deleted:
   Diag(NBA->getLocation(),
diag::err_attribute_no_builtin_on_defaulted_deleted_function)
   << NBA->getSpelling();
   break;
-case FDK_Declaration:
+case FDK::Declaration:
   Diag(NBA->getLocation(), diag::err_attribute_no_builtin_on_non_definition)
   << NBA->getSpelling();
   break;
-case FDK_Definition:
+case FDK::Definition:
   break;
 }
 
@@ -13786,7 +13786,7 @@
 ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScop

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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

[Faisal Vali via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv added a comment.

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




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

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!


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

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

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added a comment.

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?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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


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

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

[Faisal Vali via Phabricator via cfe-commits]

faisalv created this revision.
faisalv added reviewers: aaron.ballman, bruno, BRevzin, wchilders.
faisalv added a project: clang.
faisalv requested review of this revision.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91035

Files:
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaType.cpp

Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3579,7 +3579,7 @@
   // 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() != FDK::Declaration ||
   !S.CurContext->isFunctionOrMethod() ||
   D.getDeclSpec().getStorageClassSpec()
 != DeclSpec::SCS_unspecified)
@@ -5039,7 +5039,7 @@
   !(S.getLangOpts().CPlusPlus &&
 (T->isDependentType() || T->isRecordType( {
 if (T->isVoidType() && !S.getLangOpts().CPlusPlus &&
-D.getFunctionDefinitionKind() == FDK_Definition) {
+D.getFunctionDefinitionKind() == FDK::Definition) {
   // [6.9.1/3] qualified void return is invalid on a C
   // function definition.  Apparently ok on declarations and
   // in C++ though (!)
@@ -5410,7 +5410,8 @@
   //   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() == FDK::Declaration) {
 bool IsBlock = false;
 for (const DeclaratorChunk &DeclType : D.type_objects()) {
   switch (DeclType.Kind) {
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5939,7 +5939,7 @@
   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(FDK::Declaration);
 Decl *BaseD = HandleDeclarator(S, D, TemplateParamLists);
 BaseD->setImplicit(true);
 if (auto *BaseTemplD = dyn_cast(BaseD))
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -5543,7 +5543,7 @@
 }
 
 Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
-  D.setFunctionDefinitionKind(FDK_Declaration);
+  D.setFunctionDefinitionKind(FDK::Declaration);
   Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());
 
   if (OriginalLexicalContext && OriginalLexicalContext->isObjCContainer() &&
@@ -9160,15 +9160,15 @@
 // 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 FDK::Declaration:
+  case FDK::Definition:
 break;
 
-  case FDK_Defaulted:
+  case FDK::Defaulted:
 NewFD->setDefaulted();
 break;
 
-  case FDK_Deleted:
+  case FDK::Deleted:
 NewFD->setDeletedAsWritten();
 break;
 }
@@ -9871,17 +9871,17 @@
   // because Sema::ActOnStartOfFunctionDef has not been called yet.
   if (const auto *NBA = NewFD->getAttr())
 switch (D.getFunctionDefinitionKind()) {
-case FDK_Defaulted:
-case FDK_Deleted:
+case FDK::Defaulted:
+case FDK::Deleted:
   Diag(NBA->getLocation(),
diag::err_attribute_no_builtin_on_defaulted_deleted_function)
   << NBA->getSpelling();
   break;
-case FDK_Declaration:
+case FDK::Declaration:
   Diag(NBA->getLocation(), diag::err_attribute_no_builtin_on_non_definition)
   << NBA->getSpelling();
   break;
-case FDK_Definition:
+case FDK::Definition:
   break;
 }
 
@@ -13786,7 +13786,7 @@
 ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
 ParentScope, D, TemplateParameterLists, Bases);
 
-  D.setFunctionDefinitionKind(FDK_Definition);
+  D.setFunctionDefinitionKind(FDK::Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
   Decl *Dcl = ActOnStartOfFunctionDef(FnBodyScope, DP, S