-
Notifications
You must be signed in to change notification settings - Fork 291
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
refactor: Use merge_sort
instead of qsort
for sorting.
#2638
Conversation
6358d6c
to
ce3a61b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2638 +/- ##
==========================================
- Coverage 73.21% 73.13% -0.09%
==========================================
Files 149 150 +1
Lines 30595 30702 +107
==========================================
+ Hits 22401 22454 +53
- Misses 8194 8248 +54 ☔ View full report in Codecov by Sentry. |
5580dfc
to
d875693
Compare
What are the advantages of merge sort over qsort in this instance? |
I guess I can explain more in that comment if needed. This PR is part of my attempt to make toxcore C code rely less on struct layouts and low level system properties and be semantically simpler. I know C coders like to bit-twiddle everything, but I think it's worth trying to avoid that as much as possible. |
As for why merge sort instead of any other algorithm: it's an easy one to implement and has deterministic time complexity. If we have a lot of mostly-sorted arrays, we should probably just sorted-insert elements rather than adding a bunch of unsorted elements and sorting afterwards. |
a17e42d
to
37be3ea
Compare
a932f6e
to
b770dec
Compare
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.
with qsort gone, are we still using anything from stdlib.h
?
Only in group.c. I've removed it from the other 2. |
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.
and onion_announce.c ?
Done. |
This change is