[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-23 Thread John Brawn via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa0320dd8d5a: Add ParsedAttrInfo::handleDeclAttribute 
(authored by john.brawn).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31342

Files:
  clang/docs/ClangPlugins.rst
  clang/docs/InternalsManual.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3661,6 +3661,20 @@
   OS << "}\n\n";
 }
 
+static void GenerateHandleDeclAttribute(const Record , raw_ostream ) {
+  // Only generate if Attr can be handled simply.
+  if (!Attr.getValueAsBit("SimpleHandler"))
+return;
+
+  // Generate a function which just converts from ParsedAttr to the Attr type.
+  OS << "virtual AttrHandling handleDeclAttribute(Sema , Decl *D,";
+  OS << "const ParsedAttr ) const {\n";
+  OS << "  D->addAttr(::new (S.Context) " << Attr.getName();
+  OS << "Attr(S.Context, Attr));\n";
+  OS << "  return AttributeApplied;\n";
+  OS << "}\n\n";
+}
+
 static bool IsKnownToGCC(const Record ) {
   // Look at the spellings for this subject; if there are any spellings which
   // claim to be known to GCC, the attribute is known to GCC.
@@ -3742,6 +3756,7 @@
 GenerateTargetRequirements(Attr, Dupes, OS);
 GenerateSpellingIndexToSemanticSpelling(Attr, OS);
 PragmaAttributeSupport.generateStrictConformsTo(*I->second, OS);
+GenerateHandleDeclAttribute(Attr, OS);
 OS << "static const ParsedAttrInfo" << I->first << " Instance;\n";
 OS << "};\n";
 OS << "const ParsedAttrInfo" << I->first << " ParsedAttrInfo" << I->first
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6747,6 +6747,8 @@
 
   switch (AL.getKind()) {
   default:
+if (AL.getInfo().handleDeclAttribute(S, D, AL) != ParsedAttrInfo::NotHandled)
+  break;
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
@@ -6769,15 +6771,9 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMips16:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MicroMips:
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMicroMips:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MipsLongCall:
 handleSimpleAttributeWithExclusions(
 S, D, AL);
@@ -6813,9 +6809,6 @@
   case ParsedAttr::AT_WebAssemblyImportName:
 handleWebAssemblyImportNameAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_IBAction:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_IBOutlet:
 handleIBOutlet(S, D, AL);
 break;
@@ -6840,9 +6833,6 @@
   case ParsedAttr::AT_AlwaysInline:
 handleAlwaysInlineAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Artificial:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_AnalyzerNoReturn:
 handleAnalyzerNoReturnAttr(S, D, AL);
 break;
@@ -6874,9 +6864,6 @@
   case ParsedAttr::AT_Constructor:
 handleConstructorAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_CXX11NoReturn:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Deprecated:
 handleDeprecatedAttr(S, D, AL);
 break;
@@ -6904,15 +6891,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_FlagEnum:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Flatten:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_SYCLKernel:
 handleSYCLKernelAttr(S, D, AL);
 break;
@@ -6948,27 +6929,9 @@
   case ParsedAttr::AT_Restrict:
 handleRestrictAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_LifetimeBound:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_MayAlias:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Mode:
 handleModeAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_NoAlias:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoCommon:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoSplitStack:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoUniqueAddress:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_NonNull:
 if (auto *PVD = dyn_cast(D))
   

[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-20 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-18 Thread John Brawn via Phabricator via cfe-commits
john.brawn updated this revision to Diff 251113.
john.brawn added a comment.

Use an enum for the return value.


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

https://reviews.llvm.org/D31342

Files:
  clang/docs/ClangPlugins.rst
  clang/docs/InternalsManual.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3661,6 +3661,20 @@
   OS << "}\n\n";
 }
 
+static void GenerateHandleDeclAttribute(const Record , raw_ostream ) {
+  // Only generate if Attr can be handled simply.
+  if (!Attr.getValueAsBit("SimpleHandler"))
+return;
+
+  // Generate a function which just converts from ParsedAttr to the Attr type.
+  OS << "virtual AttrHandling handleDeclAttribute(Sema , Decl *D,";
+  OS << "const ParsedAttr ) const {\n";
+  OS << "  D->addAttr(::new (S.Context) " << Attr.getName();
+  OS << "Attr(S.Context, Attr));\n";
+  OS << "  return AttributeApplied;\n";
+  OS << "}\n\n";
+}
+
 static bool IsKnownToGCC(const Record ) {
   // Look at the spellings for this subject; if there are any spellings which
   // claim to be known to GCC, the attribute is known to GCC.
@@ -3742,6 +3756,7 @@
 GenerateTargetRequirements(Attr, Dupes, OS);
 GenerateSpellingIndexToSemanticSpelling(Attr, OS);
 PragmaAttributeSupport.generateStrictConformsTo(*I->second, OS);
+GenerateHandleDeclAttribute(Attr, OS);
 OS << "static const ParsedAttrInfo" << I->first << " Instance;\n";
 OS << "};\n";
 OS << "const ParsedAttrInfo" << I->first << " ParsedAttrInfo" << I->first
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6734,6 +6734,8 @@
 
   switch (AL.getKind()) {
   default:
+if (AL.getInfo().handleDeclAttribute(S, D, AL) != ParsedAttrInfo::NotHandled)
+  break;
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
@@ -6756,15 +6758,9 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMips16:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MicroMips:
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMicroMips:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MipsLongCall:
 handleSimpleAttributeWithExclusions(
 S, D, AL);
@@ -6800,9 +6796,6 @@
   case ParsedAttr::AT_WebAssemblyImportName:
 handleWebAssemblyImportNameAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_IBAction:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_IBOutlet:
 handleIBOutlet(S, D, AL);
 break;
@@ -6827,9 +6820,6 @@
   case ParsedAttr::AT_AlwaysInline:
 handleAlwaysInlineAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Artificial:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_AnalyzerNoReturn:
 handleAnalyzerNoReturnAttr(S, D, AL);
 break;
@@ -6861,9 +6851,6 @@
   case ParsedAttr::AT_Constructor:
 handleConstructorAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_CXX11NoReturn:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Deprecated:
 handleDeprecatedAttr(S, D, AL);
 break;
@@ -6891,15 +6878,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_FlagEnum:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Flatten:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_SYCLKernel:
 handleSYCLKernelAttr(S, D, AL);
 break;
@@ -6935,27 +6916,9 @@
   case ParsedAttr::AT_Restrict:
 handleRestrictAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_LifetimeBound:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_MayAlias:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Mode:
 handleModeAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_NoAlias:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoCommon:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoSplitStack:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoUniqueAddress:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_NonNull:
 if (auto *PVD = dyn_cast(D))
   handleNonNullAttrParameter(S, PVD, AL);
@@ -6974,9 +6937,6 @@
   case ParsedAttr::AT_AllocAlign:
 

[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-14 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
   break;
 }
 S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
 << AL << D->getLocation();

john.brawn wrote:
> aaron.ballman wrote:
> > john.brawn wrote:
> > > aaron.ballman wrote:
> > > > If we're in the situation where we parse a plugin-based attribute (so 
> > > > `AL.getKind()` falls into the `default` label) and its 
> > > > `handleDeclAttributes()` (incorrectly) returns false rather than true, 
> > > > it seems like we would not want to trigger either of these tests, 
> > > > correct? I'm thinking about a case where the plugin knows about that 
> > > > attribute but cannot apply it because of some other issues (maybe the 
> > > > subject is wrong, or args are incorrect, etc) and returns `false` here. 
> > > > We wouldn't want to assert or claim that this is an invalid statement 
> > > > attribute applied to a decl in that case.
> > > > 
> > > > Rather than requiring the user to return true from 
> > > > `handleDeclAttribute()`, it seems that what we really want is something 
> > > > more like:
> > > > ```
> > > > if (AL.getInfo().hasHandlerFunc()) {
> > > >   AL.getInfo().handleDeclAttribute(S, D, AL);
> > > >   break;
> > > > }
> > > > if (!AL.isStmtAttr()) {
> > > >   ...
> > > > }
> > > > S.Diag(...);
> > > > ```
> > > > where an unknown attribute does not have a handler, but simple and 
> > > > plugin attributes do have a handler. This way the user doesn't have the 
> > > > chance to accidentally get confused about what it means to handle an 
> > > > attribute. WDYT?
> > > That sounds a little overly complex, as instead of defining one thing you 
> > > now have to define two things so you have the possibility of defining one 
> > > but not the other which won't do anything useful.
> > I was hoping we could do it in such a way that the plugin user defines 
> > `handleDeclAttribute()` and the `hasHandlerFunc()` logic is automatic. This 
> > way, the user just has to define their function and "everything just works".
> > 
> > My big concern here is that we have a function with a useless return value 
> > (from the plugin author's perspective) but they're likely to get the 
> > behavior wrong through simple misunderstanding. Unless the plugin author is 
> > careful with their testing, this will lead to failed assertions when we go 
> > to check for whether it's type attribute or not.
> I don't think there's any straightforward way that hasHanderFunc could be 
> implemented automatically. The obvious choice of
> 
> ```
>   return  == ::handleDeclAttribute;
> ```
> doesn't work as  is only allowed if X.Y is an lvalue, which isn't the 
> case for a member function.
> 
> We could do something by use of a template:
> 
> ```
> class ParsedAttrInfo {
> public:
>   virtual bool hasHandlerFunc() = 0;
>   virtual void handleDeclAttribute() { }
> };
> 
> template class ParsedAttrInfoTemplate : public ParsedAttrInfo {
> public:
>   virtual bool hasHandlerFunc() {
> return ::handleDeclAttribute != ::handleDeclAttribute;
>   }
> };
> 
> class ExampleAttribute : public ParsedAttrInfoTemplate {
> public:
>   virtual void handleDeclAttribute() { }
> };
> 
> void call_fn(ParsedAttrInfo *p) {
>   if (p->hasHandlerFunc()) {
> p->handleDeclAttribute();
>   }
> }
> ```
> This works, but now we have to be careful to inherit from 
> ParsedAttrInfoTemplate in the right way.
Thank you for investigating that as a possible solution -- I agree that it 
doesn't seem too feasible. However, my concern still stands that this 
particular bit of code is too fragile and can lead to mysterious behavior for 
plugin authors pretty easily.

What about a tri-state boolean return value (aka, an enum)? The default 
`ParsedAttrInfo::handleDeclAttribute` function could return "attribute not 
known", and the other two states could be "attribute applied" or "attribute not 
applied". The logic here would then become something like:
```
if (AL.getInfo().handleDeclAttribute(S, D, AL) != AttributeNotKnown)
  break;
if (!AL.isStmtAttr()) {
  // Type attributes are handled elsewhere; silently move on.
  assert(AL.isTypeAttr() && "Non-type attribute not handled");
  break;
}
S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
<< AL << D->getLocation();
```


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-10 Thread John Brawn via Phabricator via cfe-commits
john.brawn marked an inline comment as done.
john.brawn added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
   break;
 }
 S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
 << AL << D->getLocation();

aaron.ballman wrote:
> john.brawn wrote:
> > aaron.ballman wrote:
> > > If we're in the situation where we parse a plugin-based attribute (so 
> > > `AL.getKind()` falls into the `default` label) and its 
> > > `handleDeclAttributes()` (incorrectly) returns false rather than true, it 
> > > seems like we would not want to trigger either of these tests, correct? 
> > > I'm thinking about a case where the plugin knows about that attribute but 
> > > cannot apply it because of some other issues (maybe the subject is wrong, 
> > > or args are incorrect, etc) and returns `false` here. We wouldn't want to 
> > > assert or claim that this is an invalid statement attribute applied to a 
> > > decl in that case.
> > > 
> > > Rather than requiring the user to return true from 
> > > `handleDeclAttribute()`, it seems that what we really want is something 
> > > more like:
> > > ```
> > > if (AL.getInfo().hasHandlerFunc()) {
> > >   AL.getInfo().handleDeclAttribute(S, D, AL);
> > >   break;
> > > }
> > > if (!AL.isStmtAttr()) {
> > >   ...
> > > }
> > > S.Diag(...);
> > > ```
> > > where an unknown attribute does not have a handler, but simple and plugin 
> > > attributes do have a handler. This way the user doesn't have the chance 
> > > to accidentally get confused about what it means to handle an attribute. 
> > > WDYT?
> > That sounds a little overly complex, as instead of defining one thing you 
> > now have to define two things so you have the possibility of defining one 
> > but not the other which won't do anything useful.
> I was hoping we could do it in such a way that the plugin user defines 
> `handleDeclAttribute()` and the `hasHandlerFunc()` logic is automatic. This 
> way, the user just has to define their function and "everything just works".
> 
> My big concern here is that we have a function with a useless return value 
> (from the plugin author's perspective) but they're likely to get the behavior 
> wrong through simple misunderstanding. Unless the plugin author is careful 
> with their testing, this will lead to failed assertions when we go to check 
> for whether it's type attribute or not.
I don't think there's any straightforward way that hasHanderFunc could be 
implemented automatically. The obvious choice of

```
  return  == ::handleDeclAttribute;
```
doesn't work as  is only allowed if X.Y is an lvalue, which isn't the case 
for a member function.

We could do something by use of a template:

```
class ParsedAttrInfo {
public:
  virtual bool hasHandlerFunc() = 0;
  virtual void handleDeclAttribute() { }
};

template class ParsedAttrInfoTemplate : public ParsedAttrInfo {
public:
  virtual bool hasHandlerFunc() {
return ::handleDeclAttribute != ::handleDeclAttribute;
  }
};

class ExampleAttribute : public ParsedAttrInfoTemplate {
public:
  virtual void handleDeclAttribute() { }
};

void call_fn(ParsedAttrInfo *p) {
  if (p->hasHandlerFunc()) {
p->handleDeclAttribute();
  }
}
```
This works, but now we have to be careful to inherit from 
ParsedAttrInfoTemplate in the right way.


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-06 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
   break;
 }
 S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
 << AL << D->getLocation();

john.brawn wrote:
> aaron.ballman wrote:
> > If we're in the situation where we parse a plugin-based attribute (so 
> > `AL.getKind()` falls into the `default` label) and its 
> > `handleDeclAttributes()` (incorrectly) returns false rather than true, it 
> > seems like we would not want to trigger either of these tests, correct? I'm 
> > thinking about a case where the plugin knows about that attribute but 
> > cannot apply it because of some other issues (maybe the subject is wrong, 
> > or args are incorrect, etc) and returns `false` here. We wouldn't want to 
> > assert or claim that this is an invalid statement attribute applied to a 
> > decl in that case.
> > 
> > Rather than requiring the user to return true from `handleDeclAttribute()`, 
> > it seems that what we really want is something more like:
> > ```
> > if (AL.getInfo().hasHandlerFunc()) {
> >   AL.getInfo().handleDeclAttribute(S, D, AL);
> >   break;
> > }
> > if (!AL.isStmtAttr()) {
> >   ...
> > }
> > S.Diag(...);
> > ```
> > where an unknown attribute does not have a handler, but simple and plugin 
> > attributes do have a handler. This way the user doesn't have the chance to 
> > accidentally get confused about what it means to handle an attribute. WDYT?
> That sounds a little overly complex, as instead of defining one thing you now 
> have to define two things so you have the possibility of defining one but not 
> the other which won't do anything useful.
I was hoping we could do it in such a way that the plugin user defines 
`handleDeclAttribute()` and the `hasHandlerFunc()` logic is automatic. This 
way, the user just has to define their function and "everything just works".

My big concern here is that we have a function with a useless return value 
(from the plugin author's perspective) but they're likely to get the behavior 
wrong through simple misunderstanding. Unless the plugin author is careful with 
their testing, this will lead to failed assertions when we go to check for 
whether it's type attribute or not.


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-06 Thread John Brawn via Phabricator via cfe-commits
john.brawn marked 3 inline comments as done.
john.brawn added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
   break;
 }
 S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
 << AL << D->getLocation();

aaron.ballman wrote:
> If we're in the situation where we parse a plugin-based attribute (so 
> `AL.getKind()` falls into the `default` label) and its 
> `handleDeclAttributes()` (incorrectly) returns false rather than true, it 
> seems like we would not want to trigger either of these tests, correct? I'm 
> thinking about a case where the plugin knows about that attribute but cannot 
> apply it because of some other issues (maybe the subject is wrong, or args 
> are incorrect, etc) and returns `false` here. We wouldn't want to assert or 
> claim that this is an invalid statement attribute applied to a decl in that 
> case.
> 
> Rather than requiring the user to return true from `handleDeclAttribute()`, 
> it seems that what we really want is something more like:
> ```
> if (AL.getInfo().hasHandlerFunc()) {
>   AL.getInfo().handleDeclAttribute(S, D, AL);
>   break;
> }
> if (!AL.isStmtAttr()) {
>   ...
> }
> S.Diag(...);
> ```
> where an unknown attribute does not have a handler, but simple and plugin 
> attributes do have a handler. This way the user doesn't have the chance to 
> accidentally get confused about what it means to handle an attribute. WDYT?
That sounds a little overly complex, as instead of defining one thing you now 
have to define two things so you have the possibility of defining one but not 
the other which won't do anything useful.


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6707
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
   break;
 }
 S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
 << AL << D->getLocation();

If we're in the situation where we parse a plugin-based attribute (so 
`AL.getKind()` falls into the `default` label) and its `handleDeclAttributes()` 
(incorrectly) returns false rather than true, it seems like we would not want 
to trigger either of these tests, correct? I'm thinking about a case where the 
plugin knows about that attribute but cannot apply it because of some other 
issues (maybe the subject is wrong, or args are incorrect, etc) and returns 
`false` here. We wouldn't want to assert or claim that this is an invalid 
statement attribute applied to a decl in that case.

Rather than requiring the user to return true from `handleDeclAttribute()`, it 
seems that what we really want is something more like:
```
if (AL.getInfo().hasHandlerFunc()) {
  AL.getInfo().handleDeclAttribute(S, D, AL);
  break;
}
if (!AL.isStmtAttr()) {
  ...
}
S.Diag(...);
```
where an unknown attribute does not have a handler, but simple and plugin 
attributes do have a handler. This way the user doesn't have the chance to 
accidentally get confused about what it means to handle an attribute. WDYT?


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-02 Thread John Brawn via Phabricator via cfe-commits
john.brawn updated this revision to Diff 247681.
john.brawn added a comment.

Update based on review comments.


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

https://reviews.llvm.org/D31342

Files:
  clang/docs/ClangPlugins.rst
  clang/docs/InternalsManual.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3583,6 +3583,20 @@
   OS << "}\n\n";
 }
 
+static void GenerateHandleDeclAttribute(const Record , raw_ostream ) {
+  // Only generate if Attr can be handled simply.
+  if (!Attr.getValueAsBit("SimpleHandler"))
+return;
+
+  // Generate a function which just converts from ParsedAttr to the Attr type.
+  OS << "virtual bool handleDeclAttribute(Sema , Decl *D,";
+  OS << "const ParsedAttr ) const {\n";
+  OS << "  D->addAttr(::new (S.Context) " << Attr.getName();
+  OS << "Attr(S.Context, Attr));\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+}
+
 static bool IsKnownToGCC(const Record ) {
   // Look at the spellings for this subject; if there are any spellings which
   // claim to be known to GCC, the attribute is known to GCC.
@@ -3664,6 +3678,7 @@
 GenerateTargetRequirements(Attr, Dupes, OS);
 GenerateSpellingIndexToSemanticSpelling(Attr, OS);
 PragmaAttributeSupport.generateStrictConformsTo(*I->second, OS);
+GenerateHandleDeclAttribute(Attr, OS);
 OS << "static const ParsedAttrInfo" << I->first << " Instance;\n";
 OS << "};\n";
 OS << "const ParsedAttrInfo" << I->first << " ParsedAttrInfo" << I->first
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6696,6 +6696,8 @@
 
   switch (AL.getKind()) {
   default:
+if (AL.getInfo().handleDeclAttribute(S, D, AL))
+  break;
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
@@ -6718,15 +6720,9 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMips16:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MicroMips:
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMicroMips:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MipsLongCall:
 handleSimpleAttributeWithExclusions(
 S, D, AL);
@@ -6762,9 +6758,6 @@
   case ParsedAttr::AT_WebAssemblyImportName:
 handleWebAssemblyImportNameAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_IBAction:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_IBOutlet:
 handleIBOutlet(S, D, AL);
 break;
@@ -6789,9 +6782,6 @@
   case ParsedAttr::AT_AlwaysInline:
 handleAlwaysInlineAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Artificial:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_AnalyzerNoReturn:
 handleAnalyzerNoReturnAttr(S, D, AL);
 break;
@@ -6823,9 +6813,6 @@
   case ParsedAttr::AT_Constructor:
 handleConstructorAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_CXX11NoReturn:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Deprecated:
 handleDeprecatedAttr(S, D, AL);
 break;
@@ -6853,15 +6840,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_FlagEnum:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Flatten:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_SYCLKernel:
 handleSYCLKernelAttr(S, D, AL);
 break;
@@ -6897,27 +6878,9 @@
   case ParsedAttr::AT_Restrict:
 handleRestrictAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_LifetimeBound:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_MayAlias:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Mode:
 handleModeAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_NoAlias:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoCommon:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoSplitStack:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoUniqueAddress:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_NonNull:
 if (auto *PVD = dyn_cast(D))
   handleNonNullAttrParameter(S, PVD, AL);
@@ -6936,9 +6899,6 @@
   case ParsedAttr::AT_AllocAlign:
 handleAllocAlignAttr(S, D, AL);
 break;
-  case 

[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-03-02 Thread John Brawn via Phabricator via cfe-commits
john.brawn marked 6 inline comments as done.
john.brawn added inline comments.



Comment at: clang/docs/ClangPlugins.rst:89
+The members of ``ParsedAttrInfo`` that a plugin attribute must define are:
+ * ``AttrKind``, which must be set to ``ParsedAttr::NoSemaHandlerAttribute``.
+ * ``Spellings``, which must be populated with the attribute syntaxes that are

aaron.ballman wrote:
> If it must always be this one specific value, why do we make users set it 
> manually instead of setting it automatically for them?
I've adjusted ParsedAttrInfo to default to NoSemaHandlerAttr.



Comment at: clang/docs/ClangPlugins.rst:105
+   target.
+
 Putting it all together

aaron.ballman wrote:
> It might also be nice to link to the example code from D31343 from the 
> documentation to help get users started. e.g., "To see a working example of 
> an attribute plugin, see clang/examples/Attribute/Attribute.cpp" with a link 
> to the git repo source viewer for that file. WDYT?
I'll add that as part of D31343.


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/docs/ClangPlugins.rst:105
+   target.
+
 Putting it all together

It might also be nice to link to the example code from D31343 from the 
documentation to help get users started. e.g., "To see a working example of an 
attribute plugin, see clang/examples/Attribute/Attribute.cpp" with a link to 
the git repo source viewer for that file. WDYT?


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/docs/ClangPlugins.rst:89
+The members of ``ParsedAttrInfo`` that a plugin attribute must define are:
+ * ``AttrKind``, which must be set to ``ParsedAttr::NoSemaHandlerAttribute``.
+ * ``Spellings``, which must be populated with the attribute syntaxes that are

If it must always be this one specific value, why do we make users set it 
manually instead of setting it automatically for them?



Comment at: clang/docs/ClangPlugins.rst:90
+ * ``AttrKind``, which must be set to ``ParsedAttr::NoSemaHandlerAttribute``.
+ * ``Spellings``, which must be populated with the attribute syntaxes that are
+   allowed and how the attribute name is spelled for each syntax.

The documentation should mention what type this is -- the example above doesn't 
name the type, just uses braced initialization, so it may be unclear how to 
specify a scope, for instance.



Comment at: clang/docs/ClangPlugins.rst:92
+   allowed and how the attribute name is spelled for each syntax.
+ * ``handleDeclAttribute``, which is the function that applies the attribute to
+   a declaration.

This returns a value, but there's no mention of what `true`/`false` means, and 
the example doesn't make it clear how one would "handle" the attribute (e.g., 
how one creates the semantic attribute and attaches it to the declaration).


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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-27 Thread John Brawn via Phabricator via cfe-commits
john.brawn updated this revision to Diff 246987.
john.brawn added a comment.

Update based on review comments.


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

https://reviews.llvm.org/D31342

Files:
  clang/docs/ClangPlugins.rst
  clang/docs/InternalsManual.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3583,6 +3583,20 @@
   OS << "}\n\n";
 }
 
+static void GenerateHandleDeclAttribute(const Record , raw_ostream ) {
+  // Only generate if Attr can be handled simply.
+  if (!Attr.getValueAsBit("SimpleHandler"))
+return;
+
+  // Generate a function which just converts from ParsedAttr to the Attr type.
+  OS << "virtual bool handleDeclAttribute(Sema , Decl *D,";
+  OS << "const ParsedAttr ) const {\n";
+  OS << "  D->addAttr(::new (S.Context) " << Attr.getName();
+  OS << "Attr(S.Context, Attr));\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+}
+
 static bool IsKnownToGCC(const Record ) {
   // Look at the spellings for this subject; if there are any spellings which
   // claim to be known to GCC, the attribute is known to GCC.
@@ -3664,6 +3678,7 @@
 GenerateTargetRequirements(Attr, Dupes, OS);
 GenerateSpellingIndexToSemanticSpelling(Attr, OS);
 PragmaAttributeSupport.generateStrictConformsTo(*I->second, OS);
+GenerateHandleDeclAttribute(Attr, OS);
 OS << "static const ParsedAttrInfo" << I->first << " Instance;\n";
 OS << "};\n";
 OS << "const ParsedAttrInfo" << I->first << " ParsedAttrInfo" << I->first
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6694,6 +6694,8 @@
 
   switch (AL.getKind()) {
   default:
+if (AL.getInfo().handleDeclAttribute(S, D, AL))
+  break;
 if (!AL.isStmtAttr()) {
   // Type attributes are handled elsewhere; silently move on.
   assert(AL.isTypeAttr() && "Non-type attribute not handled");
@@ -6716,15 +6718,9 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMips16:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MicroMips:
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMicroMips:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MipsLongCall:
 handleSimpleAttributeWithExclusions(
 S, D, AL);
@@ -6760,9 +6756,6 @@
   case ParsedAttr::AT_WebAssemblyImportName:
 handleWebAssemblyImportNameAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_IBAction:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_IBOutlet:
 handleIBOutlet(S, D, AL);
 break;
@@ -6787,9 +6780,6 @@
   case ParsedAttr::AT_AlwaysInline:
 handleAlwaysInlineAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Artificial:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_AnalyzerNoReturn:
 handleAnalyzerNoReturnAttr(S, D, AL);
 break;
@@ -6821,9 +6811,6 @@
   case ParsedAttr::AT_Constructor:
 handleConstructorAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_CXX11NoReturn:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Deprecated:
 handleDeprecatedAttr(S, D, AL);
 break;
@@ -6851,15 +6838,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_FlagEnum:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Flatten:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_SYCLKernel:
 handleSYCLKernelAttr(S, D, AL);
 break;
@@ -6895,27 +6876,9 @@
   case ParsedAttr::AT_Restrict:
 handleRestrictAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_LifetimeBound:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_MayAlias:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Mode:
 handleModeAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_NoAlias:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoCommon:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoSplitStack:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoUniqueAddress:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_NonNull:
 if (auto *PVD = dyn_cast(D))
   handleNonNullAttrParameter(S, PVD, AL);
@@ -6934,9 +6897,6 @@
   case ParsedAttr::AT_AllocAlign:
 handleAllocAlignAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Overloadable:
-

[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a subscriber: rsmith.
aaron.ballman added a comment.

Adding @rsmith for questions about stability guarantees.




Comment at: clang/docs/ClangPlugins.rst:74
+
+  class ExampleAttrInfo : public ParsedAttrInfo {
+  public:

john.brawn wrote:
> aaron.ballman wrote:
> > We should be documenting the fields of `ParsedAttrInfo` that the user would 
> > be interacting with. We should also consider whether we are introducing a 
> > new stability guarantee with that interface and document accordingly. I 
> > think we should not promise that this structure will be stable from release 
> > to release, but it will be stable within a given release (e.g., the type 
> > can change between Clang 11 -> Clang 12, but the type will not change 
> > between Clang 11 -> Clang 11.0.1).
> I'll add documentation of the relevant fields. As to API stability, looking 
> at Tooling.rst it says only libclang has a stable API (or rather, it says 
> libclang has a stable API, says libtooling doesn't, and says nothing about 
> plugins).
We may want to bump this out into a larger discussion and it shouldn't impact 
the ability to move forward with this patch while we discuss.

I think that for practical purposes we should be guaranteeing ABI stability for 
plugins during a patch releases (10.0 -> 10.0.1) or otherwise patch releases 
become a major compiler upgrade as opposed to an incremental one. I thought 
that we had already promised such stability for things like the AST 
serialization format for similar reasons, but maybe I'm mis-remembering. 
@rsmith -- do you have thoughts?

To be clear about the situation I'm concerned with -- it would be a shame to 
have a working attribute plugin in Clang 10.0 that either fails to load or 
silently behaves differently in Clang 10.0.1 because we generally want 
developers to be able to move to the latest patch release without undue burden.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-27 Thread John Brawn via Phabricator via cfe-commits
john.brawn marked 2 inline comments as done.
john.brawn added inline comments.



Comment at: clang/docs/ClangPlugins.rst:74
+
+  class ExampleAttrInfo : public ParsedAttrInfo {
+  public:

aaron.ballman wrote:
> We should be documenting the fields of `ParsedAttrInfo` that the user would 
> be interacting with. We should also consider whether we are introducing a new 
> stability guarantee with that interface and document accordingly. I think we 
> should not promise that this structure will be stable from release to 
> release, but it will be stable within a given release (e.g., the type can 
> change between Clang 11 -> Clang 12, but the type will not change between 
> Clang 11 -> Clang 11.0.1).
I'll add documentation of the relevant fields. As to API stability, looking at 
Tooling.rst it says only libclang has a stable API (or rather, it says libclang 
has a stable API, says libtooling doesn't, and says nothing about plugins).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6702
 
+  if (AL.getInfo().handleDeclAttribute(S, D, AL))
+return;
+

aaron.ballman wrote:
> I think this might make more logical sense as part of the `default` label in 
> the `switch` statement.
Sounds reasonable, I'll do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-02-02 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/docs/ClangPlugins.rst:74
+
+  class ExampleAttrInfo : public ParsedAttrInfo {
+  public:

We should be documenting the fields of `ParsedAttrInfo` that the user would be 
interacting with. We should also consider whether we are introducing a new 
stability guarantee with that interface and document accordingly. I think we 
should not promise that this structure will be stable from release to release, 
but it will be stable within a given release (e.g., the type can change between 
Clang 11 -> Clang 12, but the type will not change between Clang 11 -> Clang 
11.0.1).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6701-6702
 
+  if (AL.getInfo().handleDeclAttribute(S, D, AL))
+return;
+

I think this might make more logical sense as part of the `default` label in 
the `switch` statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31342



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


[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

2020-01-21 Thread John Brawn via Phabricator via cfe-commits
john.brawn updated this revision to Diff 239299.
john.brawn edited the summary of this revision.
john.brawn added reviewers: erichkeane, aaron.ballman, rjmccall.
john.brawn set the repository for this revision to rG LLVM Github Monorepo.
john.brawn added a subscriber: cfe-commits.
john.brawn added a comment.
Herald added a project: clang.

Resurrecting this old patch, plus take the opportunity to remove the need for 
some boilerplate code. This is the third of four patches to make it possible 
for clang plugins to define attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D31342

Files:
  clang/docs/ClangPlugins.rst
  clang/docs/InternalsManual.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3576,6 +3576,20 @@
   OS << "}\n\n";
 }
 
+static void GenerateHandleDeclAttribute(const Record , raw_ostream ) {
+  // Only generate if Attr can be handled simply.
+  if (!Attr.getValueAsBit("SimpleHandler"))
+return;
+
+  // Generate a function which just converts from ParsedAttr to the Attr type.
+  OS << "virtual bool handleDeclAttribute(Sema , Decl *D,";
+  OS << "const ParsedAttr ) const {\n";
+  OS << "  D->addAttr(::new (S.Context) " << Attr.getName();
+  OS << "Attr(S.Context, Attr));\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+}
+
 static bool IsKnownToGCC(const Record ) {
   // Look at the spellings for this subject; if there are any spellings which
   // claim to be known to GCC, the attribute is known to GCC.
@@ -3657,6 +3671,7 @@
 GenerateTargetRequirements(Attr, Dupes, SS);
 GenerateSpellingIndexToSemanticSpelling(Attr, SS);
 PragmaAttributeSupport.generateStrictConformsTo(*I->second, SS);
+GenerateHandleDeclAttribute(Attr, SS);
 SS << "};\n";
 SS << "static ParsedAttrInfoRegistry::Add<" << AttrInfoName << "> ";
 SS << AttrInfoName << "Instance(\"" << AttrName << "\",\"\");\n";
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6698,6 +6698,9 @@
   if (handleCommonAttributeFeatures(S, D, AL))
 return;
 
+  if (AL.getInfo().handleDeclAttribute(S, D, AL))
+return;
+
   switch (AL.getKind()) {
   default:
 if (!AL.isStmtAttr()) {
@@ -6722,15 +6725,9 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMips16:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MicroMips:
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
-  case ParsedAttr::AT_NoMicroMips:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_MipsLongCall:
 handleSimpleAttributeWithExclusions(
 S, D, AL);
@@ -6766,9 +6763,6 @@
   case ParsedAttr::AT_WebAssemblyImportName:
 handleWebAssemblyImportNameAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_IBAction:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_IBOutlet:
 handleIBOutlet(S, D, AL);
 break;
@@ -6793,9 +6787,6 @@
   case ParsedAttr::AT_AlwaysInline:
 handleAlwaysInlineAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Artificial:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_AnalyzerNoReturn:
 handleAnalyzerNoReturnAttr(S, D, AL);
 break;
@@ -6827,9 +6818,6 @@
   case ParsedAttr::AT_Constructor:
 handleConstructorAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_CXX11NoReturn:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Deprecated:
 handleDeprecatedAttr(S, D, AL);
 break;
@@ -6857,15 +6845,9 @@
   case ParsedAttr::AT_OptimizeNone:
 handleOptimizeNoneAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_FlagEnum:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_EnumExtensibility:
 handleEnumExtensibilityAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_Flatten:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_SYCLKernel:
 handleSYCLKernelAttr(S, D, AL);
 break;
@@ -6901,27 +6883,9 @@
   case ParsedAttr::AT_Restrict:
 handleRestrictAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_LifetimeBound:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_MayAlias:
-handleSimpleAttribute(S, D, AL);
-break;
   case ParsedAttr::AT_Mode:
 handleModeAttr(S, D, AL);
 break;
-  case ParsedAttr::AT_NoAlias:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoCommon:
-handleSimpleAttribute(S, D, AL);
-break;
-  case ParsedAttr::AT_NoSplitStack:
-