-
Notifications
You must be signed in to change notification settings - Fork 58
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 default_sorter #168
Milestone
Comments
Morwenn
added a commit
that referenced
this issue
Jul 18, 2020
Morwenn
added a commit
that referenced
this issue
Aug 18, 2020
The specialization stable_adapter<default_sorter> triggered a deprecation warning when default_sorter.h was included, even when the class was not otherwise used. This commit defines the specialization before default_sorter itself to avoid that warning.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
default_sorter
is wasteful and arguably not a good default, it would be better to remove it in cpp-sort 2.0.0, here are a few reasons why:std::sort
or pdqsort are good defaults and don't require dragging a whole template-heavy library as a dependency.pdq_sorter
for random-access-iterators orquick_sorter
for other iterators, which means that the complexity degrades from O(n log n) to O(n²) when the iterators are not random-access. It's quite a hard difference in some scenarios and not always acceptable for a default.low_comparisons_sorter
is used, which now feels kind of arbitrary (which specifically that one), but also super expensive!stable_adapter
,default_sorter
simply usesmerge_sorter
, which is a different algorithm with a different space complexity altogether. The difference might not matter much forstd::sort
/std::stable_sort
since those are definitely presented as different algorithms, but this is cpp-sort and one should expectstable_adapter
to actually apply its usual rules and not to switch to a wholly different algorithm.self_sort_adapter
, which is actually something you probably want to decide yourself, and not have as a default and not understand why the errors are sometimes coming from a different place. And I just noticed it but itsstable_adapter
specialization does not useself_sort_adapter
, which both surprising and a bug.cppsort::sort
andcppsort::stable_sort
, but I also plan to get rid of those in version 2.0.0 (Remove cppsort::sort and cppsort::stable_sort #167).We are pretty much done for the design decision of killing
default_sorter
, however there are other down-to-earth reasons to remove it: the deep nesting of several sorters and adapters means that it is slow to compile and produces hard to read error messages, which is something we want to avoid (see #125 and #28) and especially so for a default sorter.Here is a flame graph generated with Clang's
-ftime-trace
showing the time spent parsing some includes of the testevery_sorter.cpp
:As can be seen,
default_sorter
takes as much time to parse as most of the other sorters combined, but it also produces a bunch of wasteful instantiations while most of its features will most likely end up unused - if not totally unwanted - in the common scenarios.Basically it is a confusing & badly designed mess, and while some aspects could be changed/fixed, it will only waste resources and developer time, and does not quite fit the design of the library. It needs to go.
However it is a rather valuable tool for the test suite thanks to its deeply nested nature, so it might be worth preserving there under another name (without the
stable_adapter
specialization). A quick roadmap:cppsort::default_sorter
[[deprecated]]
in branch 1.xThe text was updated successfully, but these errors were encountered: