-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
ADL-proof implementation of [alg.merge], [alg.set.operations], [alg.heap.operations], and [alg.permutation.generators] #4347
Conversation
(void) std::set_symmetric_difference(varr, varr, varr, varr, varr3); | ||
(void) std::set_symmetric_difference(iarr, iarr, iarr, iarr, iarr3, validating_less{}); | ||
|
||
std::push_heap(varr3, varr3 + 1); // requires Cpp17ValueSwappable |
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.
It seems that MSVC STL's std::push_heap
, std::pop_heap
, std::make_heap
, and std::sort_heap
don't require Cpp17ValueSwappable (i.e. don't use ADL-found swap
) currently. Should we keep these test cases?
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.
Nor do libstdc++'s and libc++'s (Godbolt link).
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 believe we should. The fact that we don't swap today doesn't mean we never will; these test cases are useful to prevent regression.
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'm fine with this for now.
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.
@CaseyCarter If I understand this correctly, if we started ADL-swapping in meow_heap
in the future, these test cases would inherently fail to compile, because ADL-swapping is incompatible with the Incomplete scenario. I'm fine with that because we'd just comment them out like the other swapping test cases.
(void) std::set_symmetric_difference(varr, varr, varr, varr, varr3); | ||
(void) std::set_symmetric_difference(iarr, iarr, iarr, iarr, iarr3, validating_less{}); | ||
|
||
std::push_heap(varr3, varr3 + 1); // requires Cpp17ValueSwappable |
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'm fine with this for now.
tests/std/tests/P2538R1_adl_proof_std_projected/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for fixing more algorithms! 📈 🛠️ 😺 |
Separated from #4004. Towards #140 and #1596.