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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
