[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Reid Kleckner via Phabricator via cfe-commits]

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