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 toxcore code C++ compatible. #143

Merged
merged 1 commit into from Sep 24, 2016

Conversation

@iphydf
Member

iphydf commented Sep 18, 2016

It is still C code, so still compatible with C compilers as well. This
change lets us see more clearly where implicit conversions occur by
making them explicit.


This change is NOT reviewable. Use Github reviews instead.


This change is Reviewable

@nbraud

This comment has been minimized.

Show comment
Hide comment
@nbraud

nbraud Sep 18, 2016

Member

Reviewed 45 of 45 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


auto_tests/dht_test.c, line 531 at r1 (raw file):

                ck_assert_msg(count == 1, "Nodes in search don't know ip of friend. %u %u %u", i, j, count);
#if 0

Why is #if 0 required for C++ compat?


other/bootstrap_daemon/src/config.c, line 367 at r1 (raw file):

    while (config_setting_length(node_list)) {
        int address_resolved;

Any specific reason for moving the definition site (and losing cons)?


toxcore/list.c, line 116 at r1 (raw file):

static int resize(BS_LIST *list, uint32_t new_size)
{
    uint8_t *data = (uint8_t *)realloc(list->data, list->element_size * new_size);

The pointer could be const, same for ids.


toxcore/Messenger.c, line 2532 at r1 (raw file):

        uint32_t myfriend, dhtfriend;

It's strange for myfriend to be a uint32.
Perhaps friend_idx?


toxcore/TCP_connection.c, line 41 at r1 (raw file):

        ? (temp_pointer = (element_type *)realloc( \
              array, \
              (num) * sizeof(element_type)), \

This can overflow, and such a macro would actually be a nice place to catch it.
(Obviously out of scope for this PR)


toxcore/tox.c, line 845 at r1 (raw file):

    if (ret == USERSTATUS_INVALID) {
        SET_ERROR_PARAMETER(error, TOX_ERR_FRIEND_QUERY_FRIEND_NOT_FOUND);
        return (TOX_USER_STATUS)(TOX_USER_STATUS_BUSY + 1);

If the convention is to return an invalid enum value, I think there should be a macro defined for it (rather than risk pandemonium once another enum value gets added).


toxcore/tox.c, line 1352 at r1 (raw file):

}

/**********GROUP CHAT FUNCTIONS: WARNING Group chats will be rewritten so this might change ************/

Is moving the code from tox_group.c needed for C++ compat?


Comments from Reviewable

Member

nbraud commented Sep 18, 2016

Reviewed 45 of 45 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


auto_tests/dht_test.c, line 531 at r1 (raw file):

                ck_assert_msg(count == 1, "Nodes in search don't know ip of friend. %u %u %u", i, j, count);
#if 0

Why is #if 0 required for C++ compat?


other/bootstrap_daemon/src/config.c, line 367 at r1 (raw file):

    while (config_setting_length(node_list)) {
        int address_resolved;

Any specific reason for moving the definition site (and losing cons)?


toxcore/list.c, line 116 at r1 (raw file):

static int resize(BS_LIST *list, uint32_t new_size)
{
    uint8_t *data = (uint8_t *)realloc(list->data, list->element_size * new_size);

The pointer could be const, same for ids.


toxcore/Messenger.c, line 2532 at r1 (raw file):

        uint32_t myfriend, dhtfriend;

It's strange for myfriend to be a uint32.
Perhaps friend_idx?


toxcore/TCP_connection.c, line 41 at r1 (raw file):

        ? (temp_pointer = (element_type *)realloc( \
              array, \
              (num) * sizeof(element_type)), \

This can overflow, and such a macro would actually be a nice place to catch it.
(Obviously out of scope for this PR)


toxcore/tox.c, line 845 at r1 (raw file):

    if (ret == USERSTATUS_INVALID) {
        SET_ERROR_PARAMETER(error, TOX_ERR_FRIEND_QUERY_FRIEND_NOT_FOUND);
        return (TOX_USER_STATUS)(TOX_USER_STATUS_BUSY + 1);

If the convention is to return an invalid enum value, I think there should be a macro defined for it (rather than risk pandemonium once another enum value gets added).


toxcore/tox.c, line 1352 at r1 (raw file):

}

/**********GROUP CHAT FUNCTIONS: WARNING Group chats will be rewritten so this might change ************/

Is moving the code from tox_group.c needed for C++ compat?


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 22, 2016

Member

I removed reviewable, so I'm going to reply in here:

  • Why is if 0 required? It's not, that's just a drive-by cleanup. #if 0 is preferred to comments to comment out blocks of code in C. The main reason is that code formatters can see them, a second reason is that enabling it is a single bit change in the source code.
  • Why move the definition and lose const? Because goto jumps over the initialisation, which is ill-formed in C++.
  • Pointers should be const. That's not required for C++, and none of the other code does this. Perhaps we can do a transformation over the code at some point and make all non-changing names const.
  • myfriend to friend_idx. Done.
  • Overflow: I don't know if it actually can, but yes, by itself that could happen. Toxcore code tends not to check for this, because "we know that it can't overflow because we have 30% of the code in our heads at all times" is how most of the code is supposed to be read. We'll be looking into those things at some point.
  • Returning invalid values: it should not return an invalid value. The value should be unspecified, and no code should rely on it being something known, but it should return valid values. Out of scope for this PR though.
  • Moving code: no, not required, but another PR was going to do that, and I wanted this PR to be compatible with it. That PR is merged now.
Member

iphydf commented Sep 22, 2016

I removed reviewable, so I'm going to reply in here:

  • Why is if 0 required? It's not, that's just a drive-by cleanup. #if 0 is preferred to comments to comment out blocks of code in C. The main reason is that code formatters can see them, a second reason is that enabling it is a single bit change in the source code.
  • Why move the definition and lose const? Because goto jumps over the initialisation, which is ill-formed in C++.
  • Pointers should be const. That's not required for C++, and none of the other code does this. Perhaps we can do a transformation over the code at some point and make all non-changing names const.
  • myfriend to friend_idx. Done.
  • Overflow: I don't know if it actually can, but yes, by itself that could happen. Toxcore code tends not to check for this, because "we know that it can't overflow because we have 30% of the code in our heads at all times" is how most of the code is supposed to be read. We'll be looking into those things at some point.
  • Returning invalid values: it should not return an invalid value. The value should be unspecified, and no code should rely on it being something known, but it should return valid values. Out of scope for this PR though.
  • Moving code: no, not required, but another PR was going to do that, and I wanted this PR to be compatible with it. That PR is merged now.
@nurupo

nurupo approved these changes Sep 24, 2016

@@ -985,24 +985,24 @@ static unsigned int ping_node_from_getnodes_ok(DHT *dht, const uint8_t *public_k
for (i = 0; i < dht->num_friends; ++i) {
bool store_ok = 0;
DHT_Friend *friend = &dht->friends_list[i];
DHT_Friend *dht_friend = &dht->friends_list[i];

This comment has been minimized.

@nurupo

nurupo Sep 24, 2016

Member

Was that rename really necessary? Might make it harder to merge iru's new branch later on.

@nurupo

nurupo Sep 24, 2016

Member

Was that rename really necessary? Might make it harder to merge iru's new branch later on.

This comment has been minimized.

@nbraud

nbraud Sep 24, 2016

Member

@nurupo Yes, friend is a C++ keyword.

@nbraud

nbraud Sep 24, 2016

Member

@nurupo Yes, friend is a C++ keyword.

This comment has been minimized.

@GrayHatter

GrayHatter Sep 27, 2016

Again, yes. Friend in the context of DHT is problematic.

Breaking merges or otherwise this is a positive change.

@GrayHatter

GrayHatter Sep 27, 2016

Again, yes. Friend in the context of DHT is problematic.

Breaking merges or otherwise this is a positive change.

@mannol

mannol approved these changes Sep 24, 2016

LGTM

@nbraud

This comment has been minimized.

Show comment
Hide comment
@nbraud

nbraud Sep 24, 2016

Member

@iphydf OK for me, I will open issues for the out-of-scope comments

Member

nbraud commented Sep 24, 2016

@iphydf OK for me, I will open issues for the out-of-scope comments

Make toxcore code C++ compatible.
It is still C code, so still compatible with C compilers as well. This
change lets us see more clearly where implicit conversions occur by
making them explicit.

@iphydf iphydf merged commit 15cb426 into TokTok:master Sep 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+2.09%) to 73.569%
Details

@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment