[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:102
+          dyn_cast_or_null<ClassTemplateDecl>(Record->getDescribedTemplate())) {
+    if (auto *Def = Record->getDefinition())
+      addGslOwnerPointerAttributeIfNotExisting<Attribute>(Context, Def);
----------------
I wonder why this is necessary. Sema should call inference on every CXXRecordDecl. Is it because of incorrect short-circuiting in `inferGslPointerAttribute` that I'm pointing out below?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:143
+    addGslOwnerPointerAttributeIfNotExisting<PointerAttr>(
+        Context, UnderlyingRecord->getCanonicalDecl());
 }
----------------
So... what's the contract for pointer and owner attributes, are they expected to be present on every declaration in the redeclaration chain, or is only one sufficient?

Seems like here we're adding the attribute only to the canonical decl of the iterator, which will lead to the same sort of issue that this patch is trying to fix.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:200
     CXXRecordDecl *Canonical = Record->getCanonicalDecl();
     if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
       return;
----------------
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.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
 
+// The iterator typedef is a DependentNameType.
+template <typename T>
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179