gribozavr added inline comments.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:200
CXXRecordDecl *Canonical = Record->getCanonicalDecl();
if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
return;
----------------
mgehre wrote:
> 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.
I see. This is a tricky contract, and ".getCanonicalDecl()" at callsites feel
somewhat random. I think it should be at least documented. However...
> 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.
I'd support that.
================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
+// The iterator typedef is a DependentNameType.
+template <typename T>
----------------
mgehre wrote:
> gribozavr wrote:
> > This test file is getting pretty long and subtle, with lots of things are
> > being mixed into one file without isolation.
> >
> > WDYT about refactoring it into a unit test that uses AST matchers to verify
> > attribute presence?
> >
> > See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples.
> >
> > Each test case would look approximately like this:
> >
> > ```
> > EXPECT_TRUE(matches(
> > "template class ...",
> > classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer)));
> > ```
> >
> > Each test case would be isolated from other tests, each test would have a
> > name (and optionally a comment) that will make it obvious what exactly is
> > being tested.
> >
> > It would be also possible to verify things like "The iterator typedef is a
> > DependentNameType" to ensure that we're testing exactly what we want to
> > test.
> I like the AST matcher approach! This test file is really hard to debug - I
> usually copy a test to its own file for debugging.
> Would you be okay with deferring the testing change to the next PR?
Of course! Thanks!
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
