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

make the majority of the callbacks stateless and add some status to a testcase #120

Merged
merged 1 commit into from
Sep 10, 2016

Conversation

cleverca22
Copy link

@cleverca22 cleverca22 commented Sep 9, 2016

This change is Reviewable

@cleverca22 cleverca22 force-pushed the stateless branch 2 times, most recently from 3160227 to 32c7ea6 Compare September 9, 2016 14:12
@iphydf
Copy link
Member

iphydf commented Sep 9, 2016

:lgtm_strong:


Reviewed 12 of 14 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


auto_tests/tox_test.c, line 662 [r1] (raw file):

        tox_iterate(tox1, &to_compare);
        tox_iterate(tox2, &to_compare);
        tox_iterate(tox3, &packet_number);

Note to other reviewers: this is incorrect (but safe, it has the same type as to_compare) in theory. In practice, it works. Let's not focus on test code quality right now though.


Comments from Reviewable

@JFreegman
Copy link
Member

:lgtm_strong:


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


Comments from Reviewable

@JFreegman
Copy link
Member

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


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 9, 2016

Reviewed 12 of 14 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


auto_tests/tox_test.c, line 925 [r2] (raw file):

        if (counter != last_count) {
            printf("got to %d\n", counter);

Shouldn't that be %u instead?


Comments from Reviewable

@GrayHatter
Copy link

:lgtm_strong:


Reviewed 12 of 14 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 10, 2016

:lgtm_strong:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@iphydf iphydf merged commit 05f474b into TokTok:master Sep 10, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016
This pull request was closed.
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.

5 participants