Skip to content
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

Remove cppsort::sort and cppsort::stable_sort #167

Closed
8 of 9 tasks
Morwenn opened this issue Jul 16, 2020 · 1 comment
Closed
8 of 9 tasks

Remove cppsort::sort and cppsort::stable_sort #167

Morwenn opened this issue Jul 16, 2020 · 1 comment
Milestone

Comments

@Morwenn
Copy link
Owner

Morwenn commented Jul 16, 2020

cppsort::sort

cppsort::sort doesn't add much value to the library: it is dead simple to use but it just morphs sorter(...) into the longer cppsort::sort(sorter, ...) and has a bunch of drawbacks that even the documentation acknowledges:

Note that there is some heavy SFINAE wizardry happening to ensure that none of the sort overloads are ambiguous. This magic has been stripped from the documentation for clarity but may contribute to highly unreadable error messages. However, there is still some ambiguity left: the overload resolution might fail if sort is given an object that satisfies both the Compare and Projection concepts.

It basically worsens the compile times, worsens the error messages, adds additional layers of SFINAE that shouldn't be, doesn't have the benefit of function objects, and doesn't offer anything significant advantage for its cost. Also it takes the sorter by const& which means that it won't play nice with mutable sorters (#104) once they are implemented. Its only redeeming quality is that it hides default_sorter when it is not given a specific sorter, but default_sorter itself should probably be nuked (TODO: new issue).

The use of cppsort::sort in many places in the test suite is probably something that slows down the compilation for little benefit, which is a bit of a shame when we are trying to reduce compile times (#125). We also strive to make error messages more readable (#28) so it becomes difficult to justify the existence of cppsort::sort.

In 2.0.0 there won't be a simple sort() function name available in namespace cppsort:: anymore so we might be able to simplify the implementation of the ADL tricks in container_aware_adapter a bit.

cppsort::stable_sort

cppsort::stable_sort is barely better: it provides a known interface when coming from the standard library, but all it does is to hide stable_adapter. While it might make the library easier to use for very beginners, this library is supposed to be used by people who want to experiment with sorting algorithms and adapters: those are the core concepts of the library so hiding them makes little sense since people who actually want to use the library will be exposed to it very soon anyway.

Moreover using stable_adapter became way easier since CTAD was introduced in C++17 - since 2.0.0 will be C++20 only this will be entirely sufficient:

auto stable_pdq_sort = stable_adapter(pdq_sort);

cppsort::stable_sort also suffers from all the same issues as cppsort::sort, but manages to be even worse with regard to mutable sorters as highlighted in the documentation:

The main difference is that they will use stable_adapter to wrap the given sorter instead of using the raw sorter. The sorter instance is actually discarded and is only used for overload resolution, so mutable sorters won't work at all.

What to do

Here is the plan for cpp-sort 2.0.0:

  • Nuke cppsort::sort
  • Nuke cppsort::stable_sort
  • Change container_aware_adapter to not rely on cppsort::sort anymore
  • Simplify the implementation of container_aware_adapter in 2.0.0 if possible
  • Mark the functions as [[deprecated]] in branch 1.x (+ option to kill the warning)
  • Update the documentation accordingly (+ deprecation warnings option)
  • Decide what to do with the tests (keep some, remove some?)
  • Remove the use of cppsort::sort in the test suite where it's not relevant in 1.x
  • Change the example in the README
@Morwenn Morwenn added this to the 2.0.0 milestone Jul 16, 2020
@Morwenn Morwenn mentioned this issue Jul 16, 2020
4 tasks
Morwenn added a commit that referenced this issue Jul 17, 2020
This is the first step to needed to deprecate and remove components from
the library (see issues #167 and #168).
Morwenn added a commit that referenced this issue Jul 17, 2020
This mainly done as part of issue #167, but also improves the situation
a bit for issues #28 and #125.
Morwenn added a commit that referenced this issue Jul 18, 2020
cppsort::sort and cppsort::stable_sort are deprecated and will be
removed in version 2.0.0. This commit drastically reduces their use in
the test suite and only keep them where they are specifically tested
because they're relevant.

This should also reduce the parsing time of the test suite: since the
header drags default_sorter along the way it tends to be heavy for tests
where a single sorter is needed.

This commit also fixes a few minor style issues or include issues that
were hidden by transitive includes along the way.
@Morwenn Morwenn modified the milestones: 2.0.0, 1.8.0 Jul 18, 2020
Morwenn added a commit that referenced this issue Sep 23, 2020
@Morwenn
Copy link
Owner Author

Morwenn commented Nov 1, 2020

After more adjustments and backports applied to the 1.x.y branch in 65d43bc, I think that we are finally done with this.

Bye bye cppsort::sort and your younger sibling cppsort::stable_sort. You were there since the this library's very first commit, but it was time to move on and make way for other design choices. I don't think you will be missed, but I'm glad you helped to start that whole thing, which eventually led to where we stand today.

@Morwenn Morwenn closed this as completed Nov 1, 2020
Morwenn added a commit that referenced this issue Jul 4, 2022
Morwenn added a commit that referenced this issue Jul 4, 2022
@Morwenn Morwenn moved this to Todo in cpp-sort 2.0.0 Jul 4, 2022
@Morwenn Morwenn moved this from Todo to Done in cpp-sort 2.0.0 Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

1 participant