rsmith 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);
----------------
ldionne wrote:
> 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.
That's almost right.
* `Inst` represents `B<int>::X` (the template declaration in the
instantiation of `B<int>`).
* `RecordInst` represents `B<int>::X<U>` (the class declaration within the
template declaration in the instantiation of `B<int>`).
* `Pattern` represents `B<T>::X<U>` (the class declaration within the
template declaration in the definition of `B<int>`).
And we don't yet have a value for `U`, so there is no `B<int>::X<int>` here.
That is:
```
template<typename T>
struct B {
template<typename U> // #1
struct X; // Pattern
};
// implicit instantiation of B<int>:
template<> struct B<int> {
template<typename U> // Inst
struct X; // RecordInst
};
```
A template-declaration can never have attributes on it (there is no syntactic
location where they can be written), so we should never be instantiating
attributes onto it. If it could, we should instantiate those attributes from
`#1`, not from `Pattern`.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3247
InstPartialSpec->setTypeAsWritten(WrittenTy);
// Check the completed partial specialization.
----------------
erik.pilkington wrote:
> 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);`?
erik's suggestion would be the right thing to do. This case is for
instantiating a class-scope partial specialization inside a template to form
another class-scope partial specialization:
```
template<typename T> struct A {
template<typename U> struct B;
template<typename U> struct ATTRS B<U*>; // PartialSpec
};
// implicit instantiation of A<int> looks like:
template<> struct A<int> {
template<typename U> struct B;
template<typename U> struct ATTRS B<U*>; // InstPartialSpec
};
```
... where we want to instantiate attributes from `PartialSpec` to
`InstPartialSpec`. Unlike in the class template case, we don't have separate
AST nodes for the template and the class within it for partial specializations
(I think our general consensus is that's a design mistake, but addressing it
would be a substantial refactoring).
================
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.
This should call `TemplateDeclInstantiator::VisitCXXRecordDecl` to create the
member class declaration when instantiating the outer class template
specialization `C<int>`. I'm not sure why that isn't instantiating the
attributes from the declaration.
Repository:
rC Clang
https://reviews.llvm.org/D51997
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
