Skip to content

[clang] visit constraint of NTTP #91842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[clang] visit constraint of NTTP #91842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,7 @@ Bug Fixes to C++ Support
initialized, rather than evaluating them as a part of the larger manifestly constant evaluated
expression.
- Fix a bug in access control checking due to dealyed checking of friend declaration. Fixes (#GH12361).
- Fix a bug on template class partial specialization due to traverse of constraint of NTTP. Fixes (#GH77377).

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/AST/StmtProfile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2257,6 +2257,10 @@ void StmtProfiler::VisitSubstNonTypeTemplateParmExpr(
const SubstNonTypeTemplateParmExpr *E) {
// Profile exactly as the replacement expression.
Visit(E->getReplacement());
if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(E->getParameter());
NTTP && NTTP->getPlaceholderTypeConstraint()) {
Visit(NTTP->getPlaceholderTypeConstraint());
}
}

void StmtProfiler::VisitFunctionParmPackExpr(const FunctionParmPackExpr *S) {
Expand Down
6 changes: 3 additions & 3 deletions clang/test/CXX/temp/temp.decls/temp.fct/temp.func.order/p6.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,14 @@ template<C T, int I> struct Y2<T*, I, I+1+1> {}; // expected-note {{partial
template<C T, C auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
struct Y3 { Y3()=delete; };
template<C T, D auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
struct Y3<T, I, W, S, U, Z...> { Y3()=delete; };
struct Y3<T, I, W, S, U, Z...> { Y3()=delete; }; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}
template<C T, E auto I, int W, A S, template<typename, auto, int, A, typename...> class U, typename... Z>
struct Y3<T, I, W, S, U, Z...> {};
struct Y3<T, I, W, S, U, Z...> {}; // expected-note {{partial specialization matches [with T = int, I = 1, W = 1, S = A{}, U = S, Z = <int>]}}

void f() {
Y1<int, 2> a;
Y2<char*, 1, 3> b; // expected-error {{ambiguous partial specializations}}
Y3<int, 1, 1, A{}, S, int> c;
Y3<int, 1, 1, A{}, S, int> c; // expected-error {{ambiguous partial specializations of 'Y3<int, 1, 1, A{}, S, int>'}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suspicious... I don't see any FIXMEs around suggesting this was not in conformance with the standards.
@erichkeane @cor3ntin any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference is D auto vs E auto
E subsumes D so the second partial specialization is more constrained and we should pick that.
So I believe the existing test is correct and we would be introducing a regression.

It is interesting to consider that GCC, EGG and MSVC all reject that code so it is possible
I am missing something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://compiler-explorer.com/z/c87771xKf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[temp.over.link]p6.3

Two template-parameters are equivalent under the following conditions:

if they declare non-type template parameters, they have equivalent types ignoring the use of type-constraints for placeholder types

Oops, I forgot I posted the standard wording on that issue, and therefore this seems reasonable..?

Copy link
Contributor Author

@jcsxky jcsxky May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the quote can explain why we should report error here. Maybe more work is required to find out where we do the ignore and how this patch impacts this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When instantiation, we are checking which one of the two partial specialization is more specialized. Obviously, the first one(auto D) is not more specialized than the second(auto E). When applied this patch, the second one is not more specialized than the first as well. This is because isSameTemplateArg return false and the result is not TemplateDeductionResult::Success.
Although we get correct result, it is not because of ignoring the type-constraint. Back to the quote, if we ignore the use of type-constraints for placeholder types, is the following example ill-formed due to their equivalent template arguments?

template <typename> constexpr bool True = true;
template <typename T> concept C = True<T>;
template <typename T> concept D = C<T> && sizeof(T) > 2;
template <typename T> concept E = D<T> && alignof(T) > 1;

template<C auto I>
struct Y3 { Y3()=delete; };

template<D auto I>
struct Y3<I> { Y3()=delete; }; 

template<E auto I>
struct Y3<I> {};

But EDG, gcc and MSVC all accept this code. So I think the existing test is rejected may not be related to the quote. WDYT? @cor3ntin @zyn0217 @erichkeane

}

// Per [temp.func.order]p6.2.2, specifically "if the function parameters that
Expand Down
19 changes: 19 additions & 0 deletions clang/test/SemaCXX/PR77377.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
// expected-no-diagnostics

template<typename T, typename U>
constexpr bool is_same_v = false;

template<typename T>
constexpr bool is_same_v<T, T> = true;

template<typename T, typename U>
concept same_as = is_same_v<T, U>;

template <auto>
struct A {};

template <same_as<int> auto p>
struct A<p> {};

A<0> a;
Loading