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

Replace make_quick_sort with qsort #397

Merged
merged 3 commits into from Jan 7, 2017
Merged

Replace make_quick_sort with qsort #397

merged 3 commits into from Jan 7, 2017

Conversation

jhert0
Copy link
Member

@jhert0 jhert0 commented Jan 6, 2017

closes #378


This change is Reviewable

@jhert0 jhert0 assigned jhert0 and iphydf and unassigned jhert0 Jan 6, 2017
@iphydf iphydf modified the milestone: v0.1.4 Jan 6, 2017
@iphydf
Copy link
Member

iphydf commented Jan 6, 2017

Remove misc_tools.h from CMakeLists.txt and toxcore/Makefile.inc.

@iphydf
Copy link
Member

iphydf commented Jan 6, 2017

Also, rebase.

@jhert0
Copy link
Member Author

jhert0 commented Jan 6, 2017

@iphydf Done.

@Diadlo
Copy link

Diadlo commented Jan 6, 2017

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxcore/Makefile.inc, line 51 at r1 (raw file):

                        ../toxcore/TCP_connection.c \
                        ../toxcore/list.c \
                        ../toxcore/list.h \

Remove \ in the last line


Comments from Reviewable

@jhert0
Copy link
Member Author

jhert0 commented Jan 6, 2017

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxcore/Makefile.inc, line 51 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Remove \ in the last line

Done.


Comments from Reviewable

@Diadlo
Copy link

Diadlo commented Jan 7, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 7, 2017

Am I confused about how qsort works? Why doesn't this code fail?


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


auto_tests/crypto_test.c, line 347 at r2 (raw file):

static int cmp(const void *a, const void *b)
{
    if (a < b) {

This doesn't work. You're comparing the pointers, but should be comparing the values pointed at by a and b, which are clock_ts. It's surprising that it passes the tests.


auto_tests/crypto_test.c, line 374 at r2 (raw file):

    }

    qsort(results, CRYPTO_TEST_MEMCMP_COUNT, sizeof(results), cmp);

It's coincidence that this works. The third argument is the element size. sizeof(results) happens to be 8 because it's a pointer. What it should really be is sizeof(*results), because that's the size of a single array element, which happens to be 8 as well.


Comments from Reviewable

@jhert0
Copy link
Member Author

jhert0 commented Jan 7, 2017

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


auto_tests/crypto_test.c, line 347 at r2 (raw file):

Previously, iphydf wrote…

This doesn't work. You're comparing the pointers, but should be comparing the values pointed at by a and b, which are clock_ts. It's surprising that it passes the tests.

Done.


auto_tests/crypto_test.c, line 374 at r2 (raw file):

Previously, iphydf wrote…

It's coincidence that this works. The third argument is the element size. sizeof(results) happens to be 8 because it's a pointer. What it should really be is sizeof(*results), because that's the size of a single array element, which happens to be 8 as well.

Done.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 7, 2017

:lgtm_strong:


Reviewed 1 of 5 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit d41bb3a into TokTok:master Jan 7, 2017
@nurupo
Copy link
Member

nurupo commented Jan 8, 2017

No squashing, huh.

That first commit really threw me off when I saw the mistakes in the commits in master branch, just to see it getting fixed in the following commit.

@nurupo
Copy link
Member

nurupo commented Jan 12, 2017

:lgtm_strong:


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace all uses of make_quick_sort with qsort
4 participants