-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[clang] check deduction consistency when partial ordering function templates #100692
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f8fd471
[clang] check deduction consistency when partial ordering function te…
mizvekov 1c6ea6e
fixup! [clang] check deduction consistency when partial ordering func…
mizvekov 5b8099e
fixup! [clang] check deduction consistency when partial ordering func…
mizvekov 9497c1d
fixup! [clang] check deduction consistency when partial ordering func…
mizvekov 4cd65d6
Merge remote-tracking branch 'origin/main' into users/mizvekov/clang-…
mizvekov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// RUN: %clang_cc1 -std=c++23 -verify %s | ||
|
||
namespace t1 { | ||
template<bool> struct enable_if { typedef void type; }; | ||
template <class T> class Foo {}; | ||
template <class X> constexpr bool check() { return true; } | ||
template <class X, class Enable = void> struct Bar {}; | ||
|
||
template<class X> void func(Bar<X, typename enable_if<check<X>()>::type>) {} | ||
// expected-note@-1 {{candidate function}} | ||
|
||
template<class T> void func(Bar<Foo<T>>) {} | ||
// expected-note@-1 {{candidate function}} | ||
|
||
void g() { | ||
func(Bar<Foo<int>>()); // expected-error {{call to 'func' is ambiguous}} | ||
} | ||
} // namespace t1 | ||
|
||
namespace t2 { | ||
template <bool> struct enable_if; | ||
template <> struct enable_if<true> { | ||
typedef int type; | ||
}; | ||
struct pair { | ||
template <int = 0> pair(int); | ||
template <class _U2, enable_if<__is_constructible(int &, _U2)>::type = 0> | ||
pair(_U2 &&); | ||
}; | ||
int test_test_i; | ||
void test() { pair{test_test_i}; } | ||
} // namespace t2 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly everyone accepts that https://godbolt.org/z/KG9cx4c6f
I think that this is the expected behavior as I am not aware of a disambiguation rule but it's worth double checking @AaronBallman @zygoloid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a similar tests where the overload are constrained (both with subsumption, and with either one having a constraint nit satisfied)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not seeing wording that would allow us to accept this, but it is interesting that we all consistently implement the same rules and accept anyway. Should this be a Core issue in order to avoid breaking working code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I think this is contrived enough that it's unlikely to affect users.
And if it does we probably want to break them anyway... I'm not even sure what the exact behavior that determines the order is here: Which do you think should be more specialized?
Breaking code seems like a least worst options that making arbitrary ordering decisions.
In general there are tons of bugs pertaining to default arguments and ordering and we should fix them.
Adding more special cases to partial ordering seem like a bad idea.
If users find issues with the direction we can reconsider or divide a transition strategy (like we did with template template parameter ordering)
And as explain in #18291 - we are not self consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion, but when 100% of implementations all do the same thing (which we do), it seems rather silly to expect implementations and users to change instead of asking whether the standard should reflect reality.
https://godbolt.org/z/johT7fva8
I don't see a reason to break user code except for conformance, but nobody conforms so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related core issue CWG2160
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after playing around a bit more... I don't think I can explain what our behavior is in practice, so maybe this is the correct approach. Certainly the code seems ambiguous to me, so I think the changes are defensible. I'm curious if @zygoloid has opinions here, but if not, I think it's fine to move forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subsumption is not checked on deduction, that's only used later during overload resolution, which we are not changing, so I am not sure that test would be relevant to this change.
Relatedly, in a callback to another discussion with @zygoloid , we currently check if deduction would produce arguments which would not satisfy the constraints, but according to the specification this should not apply to partial ordering, which we fail to do so before this patch, and this patch introduces another case where we fail to follow the rules.