tahonermann marked 2 inline comments as done.
tahonermann 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);
----------------
rnk wrote:
> `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);
> ...
> ```
Sounds good, I'll make that change.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+ Specialization->addAttr(PrevDecl->getAttr<MSInheritanceAttr>());
+ Consumer.AssignInheritanceModel(Specialization);
+ }
----------------
rnk wrote:
> 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.
Great, thank you for confirming that.
================
Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp:958
+extern template class a<int>;
+template class a<int>;
+}
----------------
rnk wrote:
> 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)?
Yes, when a pointer to member is formed for an incomplete class, in the absence
of a `#pragma pointers_to_members` directive, use of inheritance keywords like
`__single_inheritance`, or use of the `/vmb` or `/vmg` options (or
equivalents), the "full_generality" / "virtual_inheritance" model is selected.
I did verify that manually.
I can add a `static_assert` so long as it follows the explicit template
instantiation definition. If it is placed earlier, then code related to
obtaining a complete type is run and that ends up avoiding the assertion
failure. See https://godbolt.org/z/qzTTfdfY1. This might indicate there is a
bug elsewhere; I find it surprising that the early `static_assert` has that
effect.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158869/new/
https://reviews.llvm.org/D158869
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
