Page MenuHomePhabricator

[Sema] DR727: Ensure we pick up enclosing template instantiation arguments for a class-scope explicit specialization.
AbandonedPublic

Authored by erik.pilkington on Sep 25 2018, 3:00 PM.

Details

Reviewers
rsmith
rjmccall
Summary

Previously, we stopped early on a class-scope explicit specialization, leading us to lose enclosing levels of template arguments. I'm not entirely convinced that this is the right fix, so any insight here would be greatly appreciated!

llvm.org/PR39031

Thanks!
Erik

Diff Detail

Event Timeline

clang/lib/Sema/SemaTemplateInstantiate.cpp
143

It looks like theres a bug here too:

template <class T> struct S {
  template <class T> void f() {}
  template <> void f<int>() {}
};

Will lead to a depth-2 MultiLevelTemplateArgumentList for f<int>, but I believe there should only be 1 level. I don't think theres any way to observe this though.

clang/test/SemaCXX/member-spec-dr727.cpp
35

This fails on TOT because clang substitutes int for Ap!

rsmith added inline comments.Oct 3 2018, 5:46 PM
clang/include/clang/AST/DeclTemplate.h
1771–1779

I think this name is confusing, given that we also have isMemberSpecialization, which is an entirely different thing.

Maybe isInstantiatedSpecialization would capture the essence here? (That is, this is a partial or explicit specialization that we instantiated from its enclosing template context rather than one that was declared in a non-template context.)

That also makes me wonder if we need to store additional state for this at all, or if we can determine this by checking whether the (first) declaration isOutOfLine().

clang/include/clang/AST/DeclTemplate.h
1771–1779

Maybe isInstantiatedSpecialization would capture the essence here? (That is, this is a partial or explicit specialization that we instantiated from its enclosing template context rather than one that was declared in a non-template context.)

But we should still probably return true for a specialization in a non-template class even though it wasn't instantiated (it doesn't really matter here, but for consistency). The new patch calls this isClassScopeSpecialization, what do you think of that name?

That also makes me wonder if we need to store additional state for this at all, or if we can determine this by checking whether the (first) declaration isOutOfLine().

Oh, good point! I think that would work too.

Address @rsmith's review comments.

erik.pilkington abandoned this revision.Apr 25 2019, 8:51 PM

Looks like @rsmith fixed this in r359266.

Looks like @rsmith fixed this in r359266.

Sorry, I completely forgot about the existence of this when working on that patch! :(

No worries! It happens, I probably should have pinged this more aggressively.