[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Dmitri Gribenko via Phabricator via cfe-commits]

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