Skip to content

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

Closed
@Morwenn

Description

@Morwenn

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    Status

    Done

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions