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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I now add the attributes to all redeclarations to make the logic a bit easier to follow.



================
Comment at: clang/lib/Sema/SemaAttr.cpp:94
 
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
-                                               /*DerefType*/ nullptr,
-                                               /*Spelling=*/0));
+  Record->addAttr(::new (Context) Attribute(SourceRange{}, Context,
+                                            /*DerefType*/ nullptr,
----------------
xazax.hun wrote:
> Doesn't the attribute have a `CreateImplicit` static method? If so, we could use that :)
Nice, didn't know that!


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
 
+// The iterator typedef is a DependentNameType.
+template <typename T>
----------------
gribozavr wrote:
> 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!
I added the some tests for this particular change to a new unittest and will migrate the rest of the tests later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66179/new/

https://reviews.llvm.org/D66179