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 &AL.handleDeclAttribute == &ParsedAttrInfo::handleDeclAttribute;
```
doesn't work as &X.Y 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 C> class ParsedAttrInfoTemplate : public ParsedAttrInfo {
public:
virtual bool hasHandlerFunc() {
return &C::handleDeclAttribute != &ParsedAttrInfo::handleDeclAttribute;
}
};
class ExampleAttribute : public ParsedAttrInfoTemplate<ExampleAttribute> {
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
