rnk added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10110-10111
+ // propagated to the new node prior to instantiation.
+ if (PrevDecl && PrevDecl->hasAttr<MSInheritanceAttr>()) {
+ Specialization->addAttr(PrevDecl->getAttr<MSInheritanceAttr>());
+ Consumer.AssignInheritanceModel(Specialization);
----------------
`hasAttr()` is O(n) in the attribute list size, so I would recommend this
pattern:
```
if (PrevDecl) {
if (const auto *A = PrevDecl->getAttr<MSInheritanceAttr>()) {
Specialization->addAttr(A);
...
```
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+ Specialization->addAttr(PrevDecl->getAttr<MSInheritanceAttr>());
+ Consumer.AssignInheritanceModel(Specialization);
+ }
----------------
tahonermann wrote:
> erichkeane wrote:
> > tahonermann wrote:
> > > I am concerned that moving the call to
> > > `Consumer.AssignInheritanceModel()` to immediately after the creation of
> > > the node, but before it is populated might be problematic. Previously,
> > > this call was still made before the node was completely constructed
> > > (e.g., before `setTemplateSpecializationKind()` is called for it). It is
> > > difficult to tell if any consumers might be dependent on more of the
> > > definition being present.
> > SO I think we still need to do this off of the definition, right? Else if
> > `PrevDecl` ends up being a declaration (followed later by a definition that
> > has the attribute), we'll end up missing it? Typically attributes are
> > 'added' by later decls, so only the 'latest one' (though attributes can't
> > be added after definition IIRC) has 'them all'.
> This handles the situation where a new node is created for either an explicit
> template instantiation declaration or definition; that matches prior
> behavior. The goal is to ensure each node, regardless of the reason for its
> creation, inherits the attribute from the previous declaration; that ensures
> that any explicitly declared attributes are checked for consistency (see
> `Sema::mergeMSInheritanceAttr()`).
>
> Note that an explicit class template specialization is not allowed to follow
> an explicit template instantiation declaration or definition.
> https://godbolt.org/z/cbvaac717.
I think it is safe to call `AssignInheritanceModel` earlier. We mostly just use
it to invalidate some clang AST -> LLVM IR type translation cache.
================
Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp:958
+extern template class a<int>;
+template class a<int>;
+}
----------------
My expectation here is that we assign the unknown inheritance model to
`a<int>`, is that right? Can you add a static_assert to that effect, or add
some CHECK lines for the structure, maybe make an alloca of type `void
(a<int>::*var)()` and check for the allocated type (it should be a struct with
a pointer with lots of i32s)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158869/new/
https://reviews.llvm.org/D158869
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
