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

Fix padding being in the wrong place in SAVED_FRIEND struct #313

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 10, 2016

This change is Reviewable

@robinlinden robinlinden changed the title Fix padding being in the wrong place. Fix padding being in the wrong place in SAVED_FRIEND struct Dec 10, 2016
@iphydf iphydf added this to the v0.1.0 milestone Dec 10, 2016
Copy link

@isotoxin isotoxin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it with my client. Not it works.

@iphydf iphydf added the P1 High priority label Dec 11, 2016
@GrayHatter
Copy link

:lgtm_strong:


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


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Dec 12, 2016

Generally looks good. The reason I'm nitpicking a bit on this code is that ideally the tests are a kind of documentation, showing how things can be done. Global variables are not how things are done, despite most of our current tests saying so.


Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


auto_tests/save_friend_test.c, line 19 at r2 (raw file):

#endif

static void set_random(Tox *m, bool (*function)(Tox *, const uint8_t *, size_t, TOX_ERR_SET_INFO *), size_t length)

Perhaps call this function setter. function is a bit too generic.


auto_tests/save_friend_test.c, line 25 at r2 (raw file):

    for (i = 0; i < length; ++i) {
        text[i] = ('A' + (rand() % 26));

This is non-portable. It won't do what you expect on EBCDIC. It also doesn't matter, because nobody ever displays or otherwise interprets these, so just use rand().


auto_tests/save_friend_test.c, line 33 at r2 (raw file):

}

static uint8_t status_to_compare[TOX_MAX_STATUS_MESSAGE_LENGTH] = { 0 };

Put these two into a struct and pass a pointer to the struct as user data.


auto_tests/save_friend_test.c, line 53 at r2 (raw file):

    uint8_t address[TOX_ADDRESS_SIZE];
    tox_self_get_address(tox1, address);
    tox_friend_add_norequest(tox2, address, NULL);

Although this technically works because the public key is a prefix of the address, it's misleading. We want the public key here.


auto_tests/save_friend_test.c, line 71 at r2 (raw file):

    tox_callback_friend_status_message(tox1, statuschange_callback);

    const uint32_t iteration_interval = tox_iteration_interval(tox1);

Also technically correct, because the iteration interval happens to always be the same, but you should really call it in every iteration: c_sleep(tox_iteration_interval(tox1));. You could also take the min of the two toxes iteration interval to be even more correct, but that doesn't really matter.


auto_tests/save_friend_test.c, line 87 at r2 (raw file):

    while (true) {
        if (memcmp(reference_name, name_to_compare, TOX_MAX_NAME_LENGTH) == 0 &&

When you got your struct with the name in it, also add a bool that says something like received_name and received_status_message that you can check here. Not really for efficiency, but these comparisons are slightly unclear.


Comments from Reviewable

@robinlinden
Copy link
Member Author

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


auto_tests/save_friend_test.c, line 19 at r2 (raw file):

Previously, iphydf wrote…

Perhaps call this function setter. function is a bit too generic.

Done.


auto_tests/save_friend_test.c, line 25 at r2 (raw file):

Previously, iphydf wrote…

This is non-portable. It won't do what you expect on EBCDIC. It also doesn't matter, because nobody ever displays or otherwise interprets these, so just use rand().

Done.


auto_tests/save_friend_test.c, line 33 at r2 (raw file):

Previously, iphydf wrote…

Put these two into a struct and pass a pointer to the struct as user data.

Done.


auto_tests/save_friend_test.c, line 53 at r2 (raw file):

Previously, iphydf wrote…

Although this technically works because the public key is a prefix of the address, it's misleading. We want the public key here.

Done.


auto_tests/save_friend_test.c, line 71 at r2 (raw file):

Previously, iphydf wrote…

Also technically correct, because the iteration interval happens to always be the same, but you should really call it in every iteration: c_sleep(tox_iteration_interval(tox1));. You could also take the min of the two toxes iteration interval to be even more correct, but that doesn't really matter.

Done.


auto_tests/save_friend_test.c, line 87 at r2 (raw file):

Previously, iphydf wrote…

When you got your struct with the name in it, also add a bool that says something like received_name and received_status_message that you can check here. Not really for efficiency, but these comparisons are slightly unclear.

Done.


Comments from Reviewable

@robinlinden robinlinden force-pushed the fix-save branch 2 times, most recently from 21fc324 to d3f503c Compare December 12, 2016 23:59
@iphydf iphydf assigned iphydf and unassigned isotoxin Dec 13, 2016
@iphydf
Copy link
Member

iphydf commented Dec 13, 2016

:lgtm_strong:


Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Dec 13, 2016

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


Comments from Reviewable

@robinlinden robinlinden force-pushed the fix-save branch 5 times, most recently from 9986e65 to 9aa5ae5 Compare December 13, 2016 00:28
Test covers saving and loading of a Tox instance with a friend added.
@iphydf iphydf merged commit 029c4fb into TokTok:master Dec 13, 2016
@iphydf iphydf mentioned this pull request Dec 13, 2016
@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

Reviewed 1 of 2 files at r2, 2 of 2 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

:lgtm_strong:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@iphydf iphydf changed the title Fix padding being in the wrong place in SAVED_FRIEND struct Fix padding being in the wrong place in SAVED_FRIEND struct Dec 14, 2016
@robinlinden robinlinden deleted the fix-save branch August 13, 2018 19:17
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants