[PATCH] D31342: Add ParsedAttrInfo::handleDeclAttribute

[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