Convert old groupchats to new API format

[JFreegman]

This change is

[iphydf]

Reviewed 1 of 11 files at r2.
Review status: 1 of 12 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1178 at r2 (raw file):

    }

    int g_num;

This is now uint32_t.

---

auto_tests/tox_test.c, line 1215 at r2 (raw file):

    int test_run = 0;

group_test_restart:

Not your change, but move all declarations except test_run and cur_time below this label. Then, don't initialise cur_time, and instead put the assignment as the labelled statement. Right now, cur_time stays the same, so retries get increasingly large numbers for time spent.

---

auto_tests/tox_test.c, line 1286 at r2 (raw file):

    for (i = 0; i < NUM_GROUP_TOX; ++i) {
        int num_peers = tox_conference_peer_count(toxes[i], 0);

s/num_peers/peer_count/g (and perhaps size_t) and I just realised it doesn't have an error code but should.

---

auto_tests/tox_test.c, line 1294 at r2 (raw file):

        if (num_peers != NUM_GROUP_TOX) {
            ++test_run;
            printf("\tError starting up the first group, going to restart this test. This is attempt %i\n", test_run);

Can you add some more diagnostics here? Something like "Error starting up the first group (peer_count = %d != %d for tox #%d) ...", peer_count, NUM_GROUP_TOX, i

---

auto_tests/tox_test.c, line 1316 at r2 (raw file):

        uint8_t title[2048];
        int ret = tox_conference_get_title_size(toxes[i], 0, NULL);

size_t

---

auto_tests/tox_test.c, line 1318 at r2 (raw file):

        int ret = tox_conference_get_title_size(toxes[i], 0, NULL);
        tox_conference_get_title(toxes[i], 0, title, NULL);
        ck_assert_msg(ret == sizeof("Gentoo") - 1, "Wrong title length");

Perhaps move this up one line, so the check happens right after the call.

---

other/apidsl/tox.in.h, line 2084 at r2 (raw file):

   * This event is triggered when a peer changes the conference title.
   *
   * if peer_number == UINT32_MAX, then author is unknown (e.g. initial joining the conference).

Capitalise "If".

---

other/apidsl/tox.in.h, line 2132 at r2 (raw file):

   * This function creates a new text conference.
   *
   * @return conference number on success, or UINT32_MAX on failure.

I really don't like the "returns defined value on failure". I think users should not rely on that, and this guarantee shouldn't be part of the public API. Overloading the return value like that is unnecessary and makes it hard to formally reason about it, since now the theoretically valid range of conference numbers is not the range of the return type, but one less than that range. You now need to always keep that in mind when talking about conference (or friend) numbers. We can keep it like this for now, because the rest of the API also does that, but I hope we can remove that guarantee before 0.1.

---

other/apidsl/tox.in.h, line 2179 at r2 (raw file):

     * Return the number of peers in the conference.
     */
    const uint32_t count(uint32_t conference_number);

I forgot an error here. This one can fail.

---

other/apidsl/tox.in.h, line 2211 at r2 (raw file):

    /**
     * Check if the current peer_number corresponds to our own.

Remove "current" or replace with "passed" or something. The word "current" indicates that it's part of a state, while actually it's an argument and can be very temporary.

---

other/apidsl/tox.in.h, line 2213 at r2 (raw file):

     * Check if the current peer_number corresponds to our own.
     *
     * @return true if the peer_number corresponds to our own.

You can remove this line. It adds no information.

---

other/apidsl/tox.in.h, line 2245 at r2 (raw file):

   *
   * @param friend_number The friend number of the friend who sent the invite.
   * @param cookie Received via the `${invite}` event.

"the ${event invite} event."

---

other/apidsl/tox.in.h, line 2272 at r2 (raw file):

    INIT_FAIL,
    /**
     * The join packet failed to send.

Why would this fail? Is it the same as SENDQ in other error lists?

---

other/apidsl/tox.in.h, line 2363 at r2 (raw file):

     * Set the conference title and broadcast it to the rest of the conference.
     *
     * title length cannot be longer than $MAX_NAME_LENGTH.

Capitalise "Title".

---

other/apidsl/tox.in.h, line 2383 at r2 (raw file):

     * to allocate for the array with the `$size` function.
     *
     * If the array is too small, the contents of chatlist will be truncated to `size`.

I don't think this is true. There is no size. Remove this line - no truncation happens. If you allocate less than the size function told you to, that's your fault and nobody can help you (UB happens).

---

other/apidsl/tox.in.h, line 2568 at r2 (raw file):

}

I'm not sure who added this newline, but I think we don't need it (there is only a single empty line at the start of class tox).

---

Comments from Reviewable

[JFreegman]

auto_tests/tox_test.c, line 1286 at r2 (raw file):

Previously, iphydf wrote…

s/num_peers/peer_count/g (and perhaps size_t) and I just realised it doesn't have an error code but should.

tox_conference_peer_count() can't fail.

---

Comments from Reviewable

[JFreegman]

other/apidsl/tox.in.h, line 2383 at r2 (raw file):

Previously, iphydf wrote…

I don't think this is true. There is no size. Remove this line - no truncation happens. If you allocate less than the size function told you to, that's your fault and nobody can help you (UB happens).

Do we want to leave this as UB or make it an error?

---

Comments from Reviewable

[JFreegman]

other/apidsl/tox.in.h, line 2272 at r2 (raw file):

Previously, iphydf wrote…

Why would this fail? Is it the same as SENDQ in other error lists?

Yes. I've just noticed that the description for all the other occurrences of SENDQ (/\* Packet queue is full. /*) is inaccurate, as this error indicates that write_cryptpacket_id() failed, which happens if a packet cannot be put in the send queue for any reason.

---

Comments from Reviewable

[iphydf]

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

---

auto_tests/tox_test.c, line 1286 at r2 (raw file):

Previously, JFreegman wrote…

tox_conference_peer_count() can't fail.

What does it return if you pass an invalid conference number?

---

other/apidsl/tox.in.h, line 2272 at r2 (raw file):

Previously, JFreegman wrote…

Yes. I've just noticed that the description for all the other occurrences of SENDQ (/* Packet queue is full. /*) is inaccurate, as this error indicates that write_cryptpacket_id() failed, which happens if a packet cannot be put in the send queue for any reason.

Ok, let's fix those another time. Keep this one as is.

---

other/apidsl/tox.in.h, line 2383 at r2 (raw file):

Previously, JFreegman wrote…

Do we want to leave this as UB or make it an error?

Leave it as UB, remove this line. But let's keep the thought in mind that we may want to change the API to support "educated guess" allocations, which is currently not supported by any function.

---

Comments from Reviewable

[JFreegman]

auto_tests/tox_test.c, line 1286 at r2 (raw file):

Previously, iphydf wrote…

What does it return if you pass an invalid conference number?

It doesn't use the conference number because I implemented the wrong function. Will fix and add an error.

---

Comments from Reviewable

[JFreegman]

Review status: 1 of 12 files reviewed at latest revision, 15 unresolved discussions.

---

auto_tests/tox_test.c, line 1178 at r2 (raw file):

Previously, iphydf wrote…

This is now uint32_t.

Done.

---

auto_tests/tox_test.c, line 1215 at r2 (raw file):

Previously, iphydf wrote…

Not your change, but move all declarations except test_run and cur_time below this label. Then, don't initialise cur_time, and instead put the assignment as the labelled statement. Right now, cur_time stays the same, so retries get increasingly large numbers for time spent.

Done. cur_time is used both to time the entire test duration including restarts, and also how long it takes each test to connect to the network. Now we use two different timestamps.

---

auto_tests/tox_test.c, line 1286 at r2 (raw file):

Previously, JFreegman wrote…

It doesn't use the conference number because I implemented the wrong function. Will fix and add an error.

Done.

---

auto_tests/tox_test.c, line 1294 at r2 (raw file):

Previously, iphydf wrote…

Can you add some more diagnostics here? Something like "Error starting up the first group (peer_count = %d != %d for tox #%d) ...", peer_count, NUM_GROUP_TOX, i

Done.

---

auto_tests/tox_test.c, line 1316 at r2 (raw file):

Previously, iphydf wrote…

size_t

Done.

---

auto_tests/tox_test.c, line 1318 at r2 (raw file):

Previously, iphydf wrote…

Perhaps move this up one line, so the check happens right after the call.

Done.

---

other/apidsl/tox.in.h, line 2084 at r2 (raw file):

Previously, iphydf wrote…

Capitalise "If".

Done.

---

other/apidsl/tox.in.h, line 2132 at r2 (raw file):

Previously, iphydf wrote…

I really don't like the "returns defined value on failure". I think users should not rely on that, and this guarantee shouldn't be part of the public API. Overloading the return value like that is unnecessary and makes it hard to formally reason about it, since now the theoretically valid range of conference numbers is not the range of the return type, but one less than that range. You now need to always keep that in mind when talking about conference (or friend) numbers. We can keep it like this for now, because the rest of the API also does that, but I hope we can remove that guarantee before 0.1.

I agree, I don't see any reason to use a defined value, and looking over nTox, it seems to encourage laziness (not using error codes and relying on the return value).

---

other/apidsl/tox.in.h, line 2179 at r2 (raw file):

Previously, iphydf wrote…

I forgot an error here. This one can fail.

Done.

---

other/apidsl/tox.in.h, line 2211 at r2 (raw file):

Previously, iphydf wrote…

Remove "current" or replace with "passed" or something. The word "current" indicates that it's part of a state, while actually it's an argument and can be very temporary.

Done.

---

other/apidsl/tox.in.h, line 2213 at r2 (raw file):

Previously, iphydf wrote…

You can remove this line. It adds no information.

Done.

---

other/apidsl/tox.in.h, line 2245 at r2 (raw file):

Previously, iphydf wrote…

"the ${event invite} event."

Done.

---

other/apidsl/tox.in.h, line 2363 at r2 (raw file):

Previously, iphydf wrote…

Capitalise "Title".

Done.

---

other/apidsl/tox.in.h, line 2568 at r2 (raw file):

Previously, iphydf wrote…

I'm not sure who added this newline, but I think we don't need it (there is only a single empty line at the start of class tox).

Done.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 12 files reviewed at latest revision, 5 unresolved discussions.

---

auto_tests/tox_test.c, line 1180 at r3 (raw file):

    uint32_t g_num;

    if ((g_num = tox_conference_join(tox, friendnumber, data, length, NULL)) == -1) {

You may want to change the -1 to UINT32_MAX as well, even though we don't really want to rely on that. Doesn't really matter, though, since the conversion from -1 is well-defined.

---

auto_tests/tox_test.c, line 1212 at r3 (raw file):

group_test_restart:
    ;

Ok, this is why I said "put the assignment as labelled statement", so you wouldn't need an empty labelled statement. Doesn't really matter though.

---

toxcore/group.c, line 1350 at r3 (raw file):

/* Get group title from groupnumber and put it in title.
 * title needs to be a valid memory location with a max_length size of at least MAX_NAME_LENGTH (128) bytes.

max_length is gone

---

toxcore/group.c, line 1935 at r3 (raw file):

    }

    return ret;

Now success is when it was sent to 0 peers?

---

Comments from Reviewable

[iphydf]

Review status: 10 of 12 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1289 at r6 (raw file):

    for (i = 0; i < NUM_GROUP_TOX; ++i) {
        unsigned long int peer_count = tox_conference_peer_count(toxes[i], 0, NULL);

size_t, and %zu instead of %lu below. See #140 for the decision.

---

auto_tests/tox_test.c, line 1361 at r6 (raw file):

        for (i = 0; i < (k - 1); ++i) {
            unsigned long int peer_count = tox_conference_peer_count(toxes[i], 0, NULL);

size_t

---

testing/irc_syncbot.c, line 124 at r6 (raw file):

    size_t namelen = tox_conference_peer_get_name_size(tox, groupnumber, friendgroupnumber, &error);

    TOX_ERR_CONFERENCE_PEER_QUERY error;

Move one line up.

---

testing/nTox.c, line 1132 at r6 (raw file):

    size_t len = tox_conference_peer_get_name_size(m, groupnumber, peernumber, &error);

    TOX_ERR_CONFERENCE_PEER_QUERY error;

Same here (move up).

---

Comments from Reviewable

[JFreegman]

Review status: 9 of 12 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1289 at r6 (raw file):

Previously, iphydf wrote…

size_t, and %zu instead of %lu below. See #140 for the decision.

Done, but uint32_t instead of size_t since that's what the function returns.

---

auto_tests/tox_test.c, line 1361 at r6 (raw file):

Previously, iphydf wrote…

size_t

Done.

---

Comments from Reviewable

[iphydf]

---

Review status: 9 of 12 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 11 files at r1, 2 of 11 files at r2, 2 of 7 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

---

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

---

Comments from Reviewable

[iphydf]

Reviewed 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

Comments from Reviewable

[Diadlo]

Reviewed 1 of 3 files at r4, 2 of 6 files at r8.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

---

toxcore/group.c, line 815 at r8 (raw file):

 * Return the size of peernumber's name.
 *
 * return 0 on success.

Seems, it's never return 0
return length of name if success ?

---

toxcore/group.c, line 832 at r8 (raw file):

    if (g->group[peernumber].nick_len == 0) {
        return 8;

Maybe use named const or comment, why it's 8?

---

toxcore/group.c, line 1077 at r8 (raw file):

    memcpy(response + 1 + sizeof(uint16_t), data, sizeof(uint16_t) + GROUP_IDENTIFIER_LENGTH);

    if (send_group_invite_packet(g_c->m, friendnumber, response, sizeof(response))) {

What about using early break? I.e. using negative condition and 2 lines after if closed bracket

---

toxcore/group.c, line 1307 at r8 (raw file):

    }

    if (title_len > MAX_NAME_LENGTH || title_len == 0) {

What the reason of moving this code? Check of this condition faster, than previous

---

toxcore/group.c, line 1892 at r8 (raw file):

    }

    if (len > MAX_GROUP_MESSAGE_DATA_LEN) {

Same as above

---

toxcore/group.h, line 199 at r8 (raw file):

 * Return the size of peernumber's name.
 *
 * return 0 on success.

Seems, it's never return 0
return length of name if success ?

---

Comments from Reviewable

[Diadlo]

Reviewed 1 of 11 files at r1, 2 of 11 files at r2, 2 of 3 files at r7, 4 of 6 files at r8.
Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

toxcore/group.c, line 858 at r8 (raw file):

    if (g->group[peernumber].nick_len == 0) {
        memcpy(name, "Tox User", 8);

What about use sizeof("Tox User") - 1 instead of 8?

---

toxcore/tox.c, line 1251 at r8 (raw file):

    int ret = add_groupchat(m->group_chat_object, GROUPCHAT_TYPE_TEXT);

    if (ret == -1) {

Why not ret < 0?

---

toxcore/tox.c, line 1260 at r8 (raw file):

}

bool tox_conference_delete(Tox *tox, uint32_t conference_number, TOX_ERR_CONFERENCE_DELETE *error)

Why you don't use TOX_ERR_CONFERENCE_DELETE as return type?

---

toxcore/tox.c, line 1297 at r8 (raw file):

        case -1:
            SET_ERROR_PARAMETER(error, TOX_ERR_CONFERENCE_PEER_QUERY_CONFERENCE_NOT_FOUND);
            return -1;

Return type is size_t which is unsigned

---

toxcore/tox.c, line 1470 at r8 (raw file):

        case -1:
            SET_ERROR_PARAMETER(error, TOX_ERR_CONFERENCE_TITLE_CONFERENCE_NOT_FOUND);
            return -1;

size_t is unsigned

---

toxcore/tox.c, line 1546 at r8 (raw file):

    if (ret == -1) {
        SET_ERROR_PARAMETER(error, TOX_ERR_CONFERENCE_GET_TYPE_CONFERENCE_NOT_FOUND);
        return -1;

IIRC, enum can be unsigned (implementation-defined), so return -1 can be incorrect

---

Comments from Reviewable

[JFreegman]

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

---

toxcore/group.c, line 815 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Seems, it's never return 0
return length of name if success ?

Fixed.

---

toxcore/group.c, line 832 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Maybe use named const or comment, why it's 8?

8 is the size of "Tox User" - the default name. This is ugly, but it's correct. Also, I want to try to keep things in the scope of the pull request.

---

toxcore/group.c, line 858 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

What about use sizeof("Tox User") - 1 instead of 8?

See above comment.

---

toxcore/group.c, line 1077 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

What about using early break? I.e. using negative condition and 2 lines after if closed bracket

This code block is logically correct and isn't part of my pull request, so it doesn't make sense to restructure it at the moment. You can make a pull request to clean it up later if you'd like.

---

toxcore/group.c, line 1307 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

What the reason of moving this code? Check of this condition faster, than previous

For consistency. Group existence is normally the first check, and always the return code -1. Speed isn't a factor here.

---

toxcore/tox.c, line 1251 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Why not ret < 0?

Because add_groupchat() never returns a value less than -1, and if it ever does in the future, that value will need to have its own check.

---

toxcore/tox.c, line 1260 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Why you don't use TOX_ERR_CONFERENCE_DELETE as return type?

That isn't the design of the API. TOX_ERR_\* is assigned to the passed error pointer to indicate the error (or lack thereof), and the function itself returns true or false to indicate whether the function call was successful or not.

---

toxcore/tox.c, line 1297 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Return type is size_t which is unsigned

The function will return (size_t) -1 which is the max value a size_t can hold on the given platform. The actual value of this number is irrelevant because the API does not specify a return value on error. In other words, users of the API should not even look at this value if the function errors.

---

toxcore/tox.c, line 1546 at r8 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

IIRC, enum can be unsigned (implementation-defined), so return -1 can be incorrect

See above. The value that this function returns on error is unspecified/doesn't matter.

---

Comments from Reviewable

[Diadlo]

---

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

---

toxcore/group.c, line 832 at r8 (raw file):

Previously, JFreegman wrote…

8 is the size of "Tox User" - the default name. This is ugly, but it's correct. Also, I want to try to keep things in the scope of the pull request.

Yes. I understand. I mean comment in code :) But if you want to leave it -- alright

---

Comments from Reviewable

[nurupo]

---

Reviewed 2 of 3 files at r7, 1 of 6 files at r8, 2 of 2 files at r9, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

toxcore/tox.c, line 1470 at r8 (raw file):

Previously, Diadlo (Diadlo) wrote…

size_t is unsigned

-1 is `SIZE_MAX`. Also, we will be specifying the return value of non-bool-returning functions as unspecified in case of errors.

---

Comments from Reviewable