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

[Louis Dionne via Phabricator via cfe-commits]

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);
----------------
rsmith wrote:
> 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`.
Thanks a lot for the explanation -- this is really helpful. It's now clear that this line should not be there.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2971
   InstD->setExternLoc(D->getExternLoc());
   InstD->setTemplateKeywordLoc(D->getTemplateKeywordLoc());
 
----------------
ldionne wrote:
> I tried a couple variations of things like:
> 
> ```
> SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, InstD, LateAttrs, StartingScope);
> ```
> 
> and 
> 
> ```
> SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, ClassTemplate->getTemplatedDecl(), LateAttrs, StartingScope);
> ```
> 
> For some reason, none of them end up instantiating the attribute on the `CXXRecordDecl` that is used to determine the mangling.
> 
So, assuming this:

```
template<class T> struct Foo {
  template<class U> struct X { };
  template<> struct ATTRS X<int> { };
};
```

Based on what @rsmith says about partial specializations, my understanding is that:

- `D` is the explicit specialization inside the general class template: `Foo<T>::X<int>`
- `InstD` is the explicit specialization inside the specialization of `Foo<int>`: `Foo<int>::X<int>`

And hence, what we want to do here is apply the attributes from `Foo<T>::X<int>` onto `Foo<int>::X<int>`:

```
SemaRef.InstantiateAttrsForDecl(TemplateArgs, D, InstD, LateAttrs, StartingScope);
```

However, like I said previously this is not sufficient and I'll just not implement this for specializations for the time being (for the sake of making progress on `no_extern_template`).


Repository:
  rC Clang

https://reviews.llvm.org/D51997