mgehre marked 4 inline comments as done.
mgehre added a comment.
I now start to wonder if we should instead put the attribute on all
declarations (instead of just the canonical). Later redeclarations
automatically inherit the attributes.
This would make both the checking easier (just check any declaration, no need
to obtain the canonical first), and remove the special
case for adding to the definition of templated record for ClassTemplateDecls.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:200
CXXRecordDecl *Canonical = Record->getCanonicalDecl();
if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
return;
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > Should this code not do this short-circuit check? It is prone to going
> > > out of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`,
> > > like it did just now.
> > >
> > > If you want to check for attribute before doing the string search, you
> > > could pass the string set into
> > > `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it
> > > should infer the attribute or not.
> > Yes, the `hasAttr` check here is an optimization to avoid the string
> > search. I don't think I can move the string search into
> > `addGslOwnerPointerAttributeIfNotExisting`, because that function is called
> > from other call sites that don't care about a set of names.
> Sorry I wasn't clear enough -- the issue that I noticed is that this check
> looks at the canonical decl, while `addGslOwnerPointerAttributeIfNotExisting`
> attaches the attribute to the decl itself -- that's what I meant by "out of
> sync".
I make sure that `addGslOwnerPointerAttributeIfNotExisting` is only called with
the canonical decl as argument, which the caller usually has around. The only
exception to this is when addGslOwnerPointerAttributeIfNotExisting calls itself
to attach an attribute to the definition of templated class.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66179/new/
https://reviews.llvm.org/D66179
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
