[PATCH] D51997: [clang] Make sure attributes on member classes are applied properly

[Louis Dionne via Phabricator via cfe-commits]

ldionne marked 2 inline comments as done.
ldionne added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1268
+  // Instantiate the attributes on both the template declaration and the associated record declaration.
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, Inst, LateAttrs, StartingScope);
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, Pattern, RecordInst, LateAttrs, StartingScope);
----------------
erik.pilkington wrote:
> Why are you instantiating the attributes onto the ClassTemplateDecl? At the very least it seems wrong to instantiate attributes from the pattern to the ClassTemplateDecl.
My understanding was that
- `Inst` represented `B<int>::X<T>` (the template)
- `RecordInst` represented `B<int>::X<int>` (the template specialization, which is a "real struct")
- `Pattern`  represented `B<T>::X<T>`

If that is right, then it seemed to make sense that both `B<int>::X<T>` and `B<int>::X<int>` should have the attribute, since `B<T>::X<T>` has it. I might be misunderstanding something though.


================
Comment at: clang/test/SemaCXX/PR38913.cpp:19
+};
+C<int>::X<int, long>* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+
----------------
erik.pilkington wrote:
> ldionne wrote:
> > This test is failing in the current iteration of the patch.
> I think the problem here is that we don't instantiate X<int, long> because it's behind the pointer, so we're just considering the primary template. Looks like we do add the attribute (with the fix above) here:
> ```
> template<class T> struct C {
>   template<class U, class V> struct X { };
>   template<class V> struct __attribute__((abi_tag("CTAG"))) X<int, V> { };
> };
> C<int>::X<int, long> c() { return 0; }
> ```
> But we don't get the right mangling, for some reason. Note that this problem is present even in the non-member case. 
> 
> I don't think attributes on specializations have particularly good support anyways, so I wouldn't really lose any sleep if we "left this to a follow-up". Maybe @rsmith has some thoughts here.
I'll drop support for specializations for now. The current patch is enough to unblock me on the `no_extern_template` attribute, which is the goal of all of this.


Repository:
  rC Clang

https://reviews.llvm.org/D51997