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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1267-1269
+  // 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);
----------------
nit: 80 chars.


================
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);
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1498
 
+  SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, Record, LateAttrs, StartingScope);
+
----------------
This line looks too long too.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3247
   InstPartialSpec->setTypeAsWritten(WrittenTy);
 
   // Check the completed partial specialization.
----------------
ldionne wrote:
> I tried adding
> 
> ```
> SemaRef.InstantiateAttrsForDecl(TemplateArgs, ClassTemplate->getTemplatedDecl(), InstPartialSpec, LateAttrs, StartingScope);
> ```
> 
> here, but just like for explicit specializations, that doesn't instantiate the attributes on the `CXXRecordDecl` used to determine mangling.
> 
You mean `SemaRef.InstantiateAttrsForDecl(TemplateArgs, PartialSpec, InstPartialSpec, LateAttrs, StartingScope);`?


================
Comment at: clang/test/SemaCXX/PR38913.cpp:19
+};
+C<int>::X<int, long>* c() { return 0; } // CHECK-DAG: @_Z1cB4CTAGv
+
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D51997