Fix and refactor partition - #2652
JohanMabille merged 10 commits into
Conversation
Sorry, something went wrong.
|
Cmake error in CI seems unrelated. Could not find a package configuration file provided by "xtl" (requested
version 0.7.5) with any of the following names:
xtlConfig.cmake
xtl-config.cmake
|
Sorry, something went wrong.
|
|
||
| template <class E, class C, class = std::enable_if_t<!xtl::is_integral<C>::value, int>> | ||
| inline auto partition(const xexpression<E>& e, const C& kth_container, std::ptrdiff_t axis = -1) | ||
| inline auto partition(const xexpression<E>& e, C kth_container, std::ptrdiff_t axis = -1) |
There was a problem hiding this comment.
This signature is not consistent with the previous overload, where C is passed by const ref. Passing it by value (to avoid the hidden copy) in the previous overload should be the way to go.
Note: the same should be applied to argpartition
Sorry, something went wrong.
There was a problem hiding this comment.
@JohanMabille Actually a copy of the kth container needs to be made in order to sort it so perhaps better to go with a universal reference.
Sorry, something went wrong.
There was a problem hiding this comment.
My point is, if you need a copy of kth_container in the implementation of partition, let's pass this argument by value so that the copy is not hidden to the user.
Sorry, something went wrong.
Co-authored-by:JohanMabille
Checklist
Description
Fix #2651 and refactor (some) of xsort.hpp.
There was a lot of duplications between different functions in this file.
This is far from what I would like to do, but still a good improvement for now!