[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

[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