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 

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>())
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:

  "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 

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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to