mgehre marked 6 inline comments as done.
mgehre 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);
----------------
gribozavr wrote:
> 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?
The contract is: A CXXRecordDecl is a Pointer/Owner if and only if its
canonical declaration has the Pointer/Owner attribute.
The analysis only checks this on concrete classes (excuse me if I'm using the
wrong term here), i.e. non-template classes or instantiation of template
classes.
During the inference, we might also put the attributes onto the templated
declaration of a ClassTemplateDecl. The Pointer/Owner attributes here will not
be used by the analysis.
We just put them there so clang will copy them to each instantiation, where
they will be used by the analysis.
From what I understand, clang copies the attributes of the definition of the
templated declaration to the instantiation.
In addition, when clang sees a redeclaration (e.g. of a template), it will also
copy the attributes from the previous/canonical (?) declaration.
In the problematic case
```
// The canonical declaration of the iterator template is not its definition.
template <typename T>
class __unordered_multiset_iterator;
template <typename T>
class __unordered_multiset_iterator {
};
template <typename T>
class unordered_multiset {
public:
using iterator = __unordered_multiset_iterator<T>;
};
static_assert(sizeof(unordered_multiset<int>::iterator), ""); // Force
instantiation.
```
clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for
`__unordered_multiset_iterator` (this is the canonical declaration, but not a
definition)
2. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for
`__unordered_multiset_iterator` (this is not the canonical declaration, but its
definition)
3. While processing the `using iterator` line, add a PointerAttr to the
canonical declaration (from point 1)
4. While instantiating `__unordered_multiset_iterator<int>`, copy attributes
from the template definition (from point 2), which are none
Result: The `ClassTemplateSpecializationDecl` for
`__unordered_multiset_iterator<int>` would not have the `PointerAttr`.
On the other hand, a swapped example
```
template <typename T>
class __unordered_multiset_iterator;
template <typename T>
class unordered_multiset {
public:
using iterator = __unordered_multiset_iterator<T>;
};
template <typename T>
class __unordered_multiset_iterator {
};
static_assert(sizeof(unordered_multiset<int>::iterator), ""); // Force
instantiation.
```
would work because clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for
`__unordered_multiset_iterator` (this is the canonical declaration, but not a
definition)
2. While processing the `using iterator` line, add a PointerAttr to the
canonical declaration (from point 1)
3. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for
`__unordered_multiset_iterator` (this is not the canonical declaration, but its
definition); copy all attributes from a previous declaration, i.e. the
PointerAttr from point 1
4. While instantiating `__unordered_multiset_iterator<int>`, copy PointerAttr
from the template definition (from point 3)
So we cannot just put the PointerAttr on the definition of the class template
because there might be none yet (like in the second example). We need to put it
on the definition in case
it was already parsed.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:200
CXXRecordDecl *Canonical = Record->getCanonicalDecl();
if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
return;
----------------
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.
================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
+// The iterator typedef is a DependentNameType.
+template <typename T>
----------------
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?
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
