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 events const and remove unused argument. #7

Merged
merged 1 commit into from Nov 5, 2018

Conversation

tvladyslav
Copy link

Reference issue TokTok#1185

@tvladyslav
Copy link
Author

@iphydf review this PR, please.

@iphydf
Copy link
Owner

iphydf commented Oct 28, 2018

Run other/astyle/format-source.

@tvladyslav
Copy link
Author

tvladyslav commented Oct 28, 2018

@iphydf , format-source fixes 16 files, but there are no complains about the code, that I've edited.

        modified:   toxav/groupav.c
        modified:   toxav/groupav.h
        modified:   toxcore/Messenger.c
        modified:   toxcore/TCP_connection.c
        modified:   toxcore/TCP_connection.h
        modified:   toxcore/group.c
        modified:   toxcore/group_announce.c
        modified:   toxcore/group_announce.h
        modified:   toxcore/group_chats.c
        modified:   toxcore/group_chats.h
        modified:   toxcore/group_connection.c
        modified:   toxcore/group_moderation.c
        modified:   toxcore/onion_announce.c
        modified:   toxcore/onion_client.c
        modified:   toxcore/tox.c
        modified:   toxcore/tox.h

@iphydf
Copy link
Owner

iphydf commented Oct 28, 2018

Ok, only toxcore/tox.h needs to be updated there.

@tvladyslav
Copy link
Author

Take a look at 4fbd899. astyle change is veeery suspicious.

@iphydf
Copy link
Owner

iphydf commented Oct 29, 2018

Ah yes, makes sense. TokTok#1173 would need to be fixed first. In the meantime, just change the callbacks to not take a user_data.

@iphydf
Copy link
Owner

iphydf commented Oct 29, 2018

E.g. https://github.com/iphydf/c-toxcore/blob/ngc-rebased-master/toxcore/tox.h#L3796 has user_data, but https://github.com/iphydf/c-toxcore/blob/ngc-rebased-master/toxcore/tox.h#L2509 does not. None of the callbacks registration functions should get a user_data parameter. The user_data should come from tox_iterate only.

@tvladyslav
Copy link
Author

Yes, I've removed user_data in the first commit. Together with assertions. Take a look :)

@iphydf
Copy link
Owner

iphydf commented Oct 29, 2018

Ok, thanks. Can you update the tests so they compile with this change?

@iphydf
Copy link
Owner

iphydf commented Oct 29, 2018

Also take a look at line 751. There are a bunch of callback registration calls there which cause user data to be "tox". Those need to also not get any user data, because the user data should come from tox_iterate.

@iphydf
Copy link
Owner

iphydf commented Oct 29, 2018

Also, update auto_tests/group_message_test.c line 30 to print state->index.

@tvladyslav
Copy link
Author

tvladyslav commented Nov 3, 2018

@iphydf how to run tests?

user@localhost:_build$ make test
Running tests...
Test project /home/user/github/c-toxcore/_build
No tests were found!!!

@robinlinden
Copy link

@crypto-universe You need to set the AUTOTEST CMake option.
$ cmake .. -DAUTOTEST=ON

@iphydf
Copy link
Owner

iphydf commented Nov 4, 2018

c-toxcore/auto_tests/group_announce_test.c:74:5: error: too many arguments to functiontox_callback_group_invitetox_callback_group_invite(toxes[1], group_invite_handler, nullptr);

These are some errors that need to be fixed. I realised that the issue specification was not very precise, so I'm going to accept this solution to it, even though the real solution is a lot more work. I'll file a separate issue for the remainder of the work.

@tvladyslav
Copy link
Author

Wait a second, in a few minutes there will be a working (at least compiling) solution.

@@ -27,7 +27,7 @@ static void group_invite_handler(Tox *tox, uint32_t friend_number, const uint8_t
self_info.nick_length = 4;
self_info.user_status = TOX_USER_STATUS_NONE;

printf("invite arrived; accepting\n");
printf("invite arrived; accepting; tox0_index=%u, tox1_index=%u\n", global_state_tox0->index, global_state_tox1->index);
Copy link
Owner

Choose a reason for hiding this comment

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

error: ‘global_state_tox0’ undeclared (first use in this function)

It'll need to move up. But also, let's not do this in this PR, because what we actually want is no use of global state in this test. If you do that, you'll notice it's quite a bit of work.

@tvladyslav
Copy link
Author

My fault. Trivial change, so I didn't check if it compiles.

@iphydf iphydf merged commit f24269e into iphydf:ngc-rebased-master Nov 5, 2018
iphydf pushed a commit that referenced this pull request Jan 3, 2019
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.

None yet

3 participants