[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
alexander-shaposhnikov updated this revision to Diff 503750.
alexander-shaposhnikov added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145034/new/
https://reviews.llvm.org/D145034
Files:
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/DeclSpec.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaCXXScopeSpec.cpp
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
clang/test/SemaTemplate/concepts-out-of-line-def.cpp
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+static constexpr int PRIMARY = 0;
+static constexpr int SPECIALIZATION_CONCEPT = 1;
+static constexpr int SPECIALIZATION_REQUIRES = 2;
+
+template
+concept Concept = (sizeof(T) >= 2 * sizeof(int));
+
+struct XY {
+ int x;
+ int y;
+};
+
+namespace members {
+
+template struct S {
+ static constexpr int primary();
+};
+
+template constexpr int S::primary() {
+ return PRIMARY;
+};
+
+template struct S {
+ static constexpr int specialization();
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+struct S {
+ static constexpr int specialization();
+};
+
+template constexpr int S::specialization() {
+ return SPECIALIZATION_CONCEPT;
+}
+
+template
+ requires(sizeof(T) == sizeof(int))
+constexpr int S::specialization() {
+ return SPECIALIZATION_REQUIRES;
+}
+
+static_assert(S::primary() == PRIMARY);
+static_assert(S::specialization() == SPECIALIZATION_CONCEPT);
+static_assert(S::specialization() == SPECIALIZATION_REQUIRES);
+
+} // namespace members
+
+namespace enumerations {
+
+template struct S {
+ enum class E : int;
+};
+
+template enum class S::E { Value = PRIMARY };
+
+template struct S {
+ enum class E : int;
+};
+
+template
+enum class S::E {
+ Value = SPECIALIZATION_CONCEPT
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+struct S {
+ enum class E : int;
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+enum class S::E {
+ Value = SPECIALIZATION_REQUIRES
+};
+
+static_assert(static_cast(S::E::Value) == PRIMARY);
+static_assert(static_cast(S::E::Value) ==
+ SPECIALIZATION_CONCEPT);
+static_assert(static_cast(S::E::Value) ==
+ SPECIALIZATION_REQUIRES);
+
+} // namespace enumerations
+
+namespace multiple_template_parameter_lists {
+
+template
+struct S {
+ template
+ static constexpr int primary(Inner);
+};
+
+template
+template
+constexpr int S::primary(Inner) {
+ return PRIMARY;
+};
+
+template
+struct S {
+ template
+ static constexpr int specialization(Inner);
+};
+
+template
+template
+constexpr int S::specialization(Inner) { return SPECIALIZATION_CONCEPT; }
+
+template
+ requires(sizeof(Outer) == sizeof(int))
+struct S {
+ template
+ static constexpr int specialization(Inner);
+};
+
+template
+ requires(sizeof(Outer) == sizeof(int))
+template
+constexpr int S::specialization(Inner) { return SPECIALIZATION_REQUIRES; }
+
+static_assert(S::primary("str") == PRIMARY);
+static_assert(S::specialization("str") == SPECIALIZATION_CONCEPT);
+static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
+
+} // namespace multiple_template_parameter_lists
Index: clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
+++ clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
@@ -3,7 +3,7 @@
template
struct A;
-template // expected-note{{previous template declaration}}
+template
struct A {
void f0();
void f1();
@@ -15,11 +15,10 @@
void g0();
};
-// FIXME: We should probably give more precise diagnostics here, but the
-// diagnostics we give aren't terrible.
-// FIXME: why not point to the first parameter that's "too many"?
-template // expected-error{{too many template parameters}}
-void A::f0() { }
+// FIXME: We should produce diagnostics pointing out the
+// non-matching candidates.
+template
+void A::f0() { } // expected-error{{does not refer into a class, class template or class template partial specialization}}
template
void A::f1() { } // expected-error{{out-of-line definition}}
Index: clang/lib/Sema/SemaCXXScopeSpec.cpp
===
--- clang/lib/Sema/SemaCXXScopeSpec.cpp
+++ clang/lib/Sema/SemaCXXScopeSpec.cpp
@@ -99,34 +99,52 @@
if (ClassTemplateDecl *ClassTemplate
= dyn_cast_or_null(
SpecType->getTemplateName().getAsTemplateDecl())) {
- QualType ContextType
-= Context.getCanonicalType(QualType(SpecType,
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
rsmith added inline comments.
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:113-114
+ if (TemplateParamLists.size() == 1) {
+// FIXME: pick the correct template parameter list based on NNS, SS
+// and getCurScope().
+TemplateParameterList *L = TemplateParamLists[0];
I think this can be done fairly straightforwardly: we should pick out the
template parameter list whose depth equals the depth of
`ClassTemplate->getTemplateParameters()`.
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:141
+ ClassTemplate->getInjectedClassNameSpecialization();
+ if (Context.hasSameType(Injected, ContextType))
+return ClassTemplate->getTemplatedDecl();
This should also be guarded by a check that `TemplateParameterListsAreEqual`
between `ClassTemplate->getTemplateParameters()` and the template parameter
list we picked out of the given array of template parameter lists.
(With that check in place, we can move this back before the search for partial
specializations.)
Comment at:
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp:2-4
+// XFAIL: *
+// NOTE: This test is marked XFAIL until the diagnostics of
+// too many template parameters is fixed.
alexander-shaposhnikov wrote:
> rsmith wrote:
> > What is the new diagnostic output? (Presumably we no longer match the
> > partial specialization when trying to match the declaration on line 25,
> > because the template parameter list doesn't match.)
> the new diagnostic output:
> error: nested name specifier 'A::' for declaration does not refer
> into a class, class template or class template partial specialization
>
> ```
> template
> void A::f0() { }
> ```
>
>
Hm, it'd definitely be nice to produce some notes pointing out the non-matching
candidates, and ideally, why they don't match. But I don't think this is so
terrible that we need to block the patch on it. I think for now you should
update the test expectations and update the FIXME below to say what we want
here, and remove the XFAIL.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145034/new/
https://reviews.llvm.org/D145034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
alexander-shaposhnikov added inline comments.
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1677-1678
CXXScopeSpec Spec;
+if (TemplateInfo.TemplateParams)
+ Spec.setTemplateParamLists(*TemplateInfo.TemplateParams);
+
rsmith wrote:
> I think we'll need some similar logic for out-of-line definitions of member
> enums too:
> ```
> template struct X;
> template concept bool Small = sizeof(T) == 1;
> template struct X {
> enum class E;
> };
> template enum class X::E { e };
> ```
>
> More generally, anywhere we might pass `EnteringContext = true`, we probably
> ought to think about whether we have template parameters to pass in.
Added missing bits for enums,
regarding EnteringContext = true - yeah, I agree, I haven't done it on this
diff cause I'm kind of worried to do it without test cases and extracting test
cases requires some efforts.
Comment at:
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp:2-4
+// XFAIL: *
+// NOTE: This test is marked XFAIL until the diagnostics of
+// too many template parameters is fixed.
rsmith wrote:
> What is the new diagnostic output? (Presumably we no longer match the partial
> specialization when trying to match the declaration on line 25, because the
> template parameter list doesn't match.)
the new diagnostic output:
error: nested name specifier 'A::' for declaration does not refer
into a class, class template or class template partial specialization
```
template
void A::f0() { }
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145034/new/
https://reviews.llvm.org/D145034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
alexander-shaposhnikov updated this revision to Diff 502067.
alexander-shaposhnikov added a comment.
1/ Add support for out-of-line definitions of member enums + add tests
2/ Updated comments DeclSpec.h
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145034/new/
https://reviews.llvm.org/D145034
Files:
clang/include/clang/Sema/DeclSpec.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaCXXScopeSpec.cpp
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
clang/test/SemaTemplate/concepts-out-of-line-def.cpp
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+static constexpr int PRIMARY = 0;
+static constexpr int SPECIALIZATION_CONCEPT = 1;
+static constexpr int SPECIALIZATION_REQUIRES = 2;
+
+template
+concept Concept = (sizeof(T) >= 2 * sizeof(int));
+
+struct XY {
+ int x;
+ int y;
+};
+
+namespace members {
+
+template struct S {
+ constexpr int primary();
+};
+
+template constexpr int S::primary() {
+ return PRIMARY;
+};
+
+template struct S {
+ constexpr int specialization();
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+struct S {
+ constexpr int specialization();
+};
+
+template constexpr int S::specialization() {
+ return SPECIALIZATION_CONCEPT;
+}
+
+template
+ requires(sizeof(T) == sizeof(int))
+constexpr int S::specialization() {
+ return SPECIALIZATION_REQUIRES;
+}
+
+static_assert(S().primary() == PRIMARY);
+static_assert(S().specialization() == SPECIALIZATION_CONCEPT);
+static_assert(S().specialization() == SPECIALIZATION_REQUIRES);
+
+} // namespace members
+
+namespace enumerations {
+
+template struct S {
+ enum class E : int;
+};
+
+template enum class S::E { Value = PRIMARY };
+
+template struct S {
+ enum class E : int;
+};
+
+template
+enum class S::E {
+ Value = SPECIALIZATION_CONCEPT
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+struct S {
+ enum class E : int;
+};
+
+template
+ requires(sizeof(T) == sizeof(int))
+enum class S::E {
+ Value = SPECIALIZATION_REQUIRES
+};
+
+static_assert(static_cast(S::E::Value) == PRIMARY);
+static_assert(static_cast(S::E::Value) ==
+ SPECIALIZATION_CONCEPT);
+static_assert(static_cast(S::E::Value) ==
+ SPECIALIZATION_REQUIRES);
+
+} // namespace enumerations
Index: clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
+++ clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
@@ -1,4 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
+// XFAIL: *
+// NOTE: This test is marked XFAIL until the diagnostics of
+// too many template parameters is fixed.
template
struct A;
Index: clang/lib/Sema/SemaCXXScopeSpec.cpp
===
--- clang/lib/Sema/SemaCXXScopeSpec.cpp
+++ clang/lib/Sema/SemaCXXScopeSpec.cpp
@@ -102,31 +102,44 @@
QualType ContextType
= Context.getCanonicalType(QualType(SpecType, 0));
- // If the type of the nested name specifier is the same as the
- // injected class name of the named class template, we're entering
- // into that class template definition.
- QualType Injected
-= ClassTemplate->getInjectedClassNameSpecialization();
- if (Context.hasSameType(Injected, ContextType))
-return ClassTemplate->getTemplatedDecl();
+ // FIXME: currently only the case of ParamLists containing a single
+ // element is supported. The fallback on the search of partial
+ // specialization using ContextType should be eventually removed since
+ // it doesn't handle the case of constrained template parameters
+ // correctly.
+ ClassTemplatePartialSpecializationDecl *PartialSpec = nullptr;
+ ArrayRef TemplateParamLists = SS.getTemplateParamLists();
+ if (TemplateParamLists.size() == 1) {
+// FIXME: pick the correct template parameter list based on NNS, SS
+// and getCurScope().
+TemplateParameterList *L = TemplateParamLists[0];
+void *Pos = nullptr;
+PartialSpec = ClassTemplate->findPartialSpecialization(
+SpecType->template_arguments(), L, Pos);
+ } else {
+PartialSpec = ClassTemplate->findPartialSpecialization(ContextType);
+ }
- // If the type of the nested name specifier is the same as the
- // type of one of the class template's class template partial
- // specializations,
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
shafik added inline comments. Comment at: clang/include/clang/Sema/DeclSpec.h:68 NestedNameSpecifierLocBuilder Builder; + ArrayRef TemplateParamLists; Does this really represent a `nested-name-specifier` with a `template-param-list`? Maybe I am misunderstanding the change but it feels like we should be updating the documentation to reflect this change better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145034/new/ https://reviews.llvm.org/D145034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
rsmith added a comment.
This direction looks promising to me.
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1677-1678
CXXScopeSpec Spec;
+if (TemplateInfo.TemplateParams)
+ Spec.setTemplateParamLists(*TemplateInfo.TemplateParams);
+
I think we'll need some similar logic for out-of-line definitions of member
enums too:
```
template struct X;
template concept bool Small = sizeof(T) == 1;
template struct X {
enum class E;
};
template enum class X::E { e };
```
More generally, anywhere we might pass `EnteringContext = true`, we probably
ought to think about whether we have template parameters to pass in.
Comment at:
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp:2-4
+// XFAIL: *
+// NOTE: This test is marked XFAIL until the diagnostics of
+// too many template parameters is fixed.
What is the new diagnostic output? (Presumably we no longer match the partial
specialization when trying to match the declaration on line 25, because the
template parameter list doesn't match.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145034/new/
https://reviews.llvm.org/D145034
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates
alexander-shaposhnikov created this revision.
alexander-shaposhnikov added reviewers: rsmith, aaron.ballman.
alexander-shaposhnikov created this object with visibility "All Users".
Herald added a project: All.
alexander-shaposhnikov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This diff starts fixing our handling of out-of-line definitions of constrained
templates.
Initially it was motivated by https://github.com/llvm/llvm-project/issues/49620
.
The current diff doesn't fully address those issues, more changes are required
to make things work for the case where
multiple template parameter lists are attached to the declaration, this will be
done in a follow-up diff.
Test plan:
1/ ninja check-all
2/ Bootstrapped Clang passes all the tests
3/ Internal testing (built a few large projects (that use templates
extensively) with the bootstrapped version of Clang)
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D145034
Files:
clang/include/clang/Sema/DeclSpec.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaCXXScopeSpec.cpp
clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
clang/test/SemaTemplate/concepts-out-of-line-def.cpp
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+static constexpr int PRIMARY = 0;
+static constexpr int SPECIALIZATION_CONCEPT = 1;
+static constexpr int SPECIALIZATION_REQUIRES = 2;
+
+template
+struct S {
+ constexpr int primary();
+};
+
+template
+constexpr int S::primary() { return PRIMARY; };
+
+template
+concept Concept = (sizeof(T) >= 2 * sizeof(int));
+
+template
+struct S {
+constexpr int specialization();
+};
+
+template
+requires (sizeof(T) == sizeof(int))
+struct S {
+constexpr int specialization();
+};
+
+
+template
+constexpr int S::specialization() { return SPECIALIZATION_CONCEPT; }
+
+
+template
+requires (sizeof(T) == sizeof(int))
+constexpr int S::specialization() { return SPECIALIZATION_REQUIRES; }
+
+struct XY {
+ int x;
+ int y;
+};
+
+static_assert (S().primary() == PRIMARY);
+static_assert (S().specialization() == SPECIALIZATION_CONCEPT);
+static_assert (S().specialization() == SPECIALIZATION_REQUIRES);
+
Index: clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
+++ clang/test/CXX/temp/temp.decls/temp.class.spec/temp.class.spec.mfunc/p1-neg.cpp
@@ -1,4 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
+// XFAIL: *
+// NOTE: This test is marked XFAIL until the diagnostics of
+// too many template parameters is fixed.
template
struct A;
Index: clang/lib/Sema/SemaCXXScopeSpec.cpp
===
--- clang/lib/Sema/SemaCXXScopeSpec.cpp
+++ clang/lib/Sema/SemaCXXScopeSpec.cpp
@@ -102,31 +102,44 @@
QualType ContextType
= Context.getCanonicalType(QualType(SpecType, 0));
- // If the type of the nested name specifier is the same as the
- // injected class name of the named class template, we're entering
- // into that class template definition.
- QualType Injected
-= ClassTemplate->getInjectedClassNameSpecialization();
- if (Context.hasSameType(Injected, ContextType))
-return ClassTemplate->getTemplatedDecl();
+ // FIXME: currently only the case of ParamLists containing a single
+ // element is supported. The fallback on the search of partial
+ // specialization using ContextType should be eventually removed since
+ // it doesn't handle the case of constrained template parameters
+ // correctly.
+ ClassTemplatePartialSpecializationDecl *PartialSpec = nullptr;
+ ArrayRef TemplateParamLists = SS.getTemplateParamLists();
+ if (TemplateParamLists.size() == 1) {
+// FIXME: pick the correct template parameter list based on NNS, SS
+// and getCurScope().
+TemplateParameterList *L = TemplateParamLists[0];
+void *Pos = nullptr;
+PartialSpec = ClassTemplate->findPartialSpecialization(
+SpecType->template_arguments(), L, Pos);
+ } else {
+PartialSpec = ClassTemplate->findPartialSpecialization(ContextType);
+ }
- // If the type of the nested name specifier is the same as the
- // type of one of the class template's class template partial
- // specializations, we're entering into the definition of that
- // class template partial
