Made save format platform-independent

[robinlinden]

Fixes #215.

---

This change is 

[iphydf]

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

---

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

{
    uint32_t size = 0;
#define SIZE_OF_FIELD(NAME) size += sizeof(temp.NAME)

You could instead copy/paste the exact COPY_VALUE/COPY_ARRAY sequence and define them as adding sizeof to a uint32_t data. That way, it is indeed copy/paste, but it's clean copy/paste. Right now it's a derived copy/paste that can get out of sync. There isn't even a comment saying that it's the same as something else.

---

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

        if (m->friendlist[i].status > 0) {
            struct SAVED_FRIEND temp;
            memset(&temp, 0, saved_friend_size());

This is now wrong. temp is in fact of size sizeof temp or sizeof(struct SAVED_FRIEND). Remove this line and change the declaration to struct SAVED_FRIEND temp = { 0 };.

---

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

    for (i = 0; i < num; ++i) {
        struct SAVED_FRIEND temp;

Add initialiser: = { 0 }.

---

Comments from Reviewable

[iphydf]

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

---

Comments from Reviewable

[robinlinden]

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

---

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

Previously, iphydf wrote…

You could instead copy/paste the exact COPY_VALUE/COPY_ARRAY sequence and define them as adding sizeof to a uint32_t data. That way, it is indeed copy/paste, but it's clean copy/paste. Right now it's a derived copy/paste that can get out of sync. There isn't even a comment saying that it's the same as something else.

Done.

---

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

Previously, iphydf wrote…

This is now wrong. temp is in fact of size sizeof temp or sizeof(struct SAVED_FRIEND). Remove this line and change the declaration to struct SAVED_FRIEND temp = { 0 };.

Done.

---

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

Previously, iphydf wrote…

Add initialiser: = { 0 }.

Done.

---

Comments from Reviewable

[iphydf]

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

---

toxcore/Messenger.c, line 2654 at r2 (raw file):

{
    uint32_t data = 0;
    struct SAVED_FRIEND *temp = { 0 };

const struct SAVED_FRIEND temp = { 0 };

---

toxcore/Messenger.c, line 2657 at r2 (raw file):

#define COPY_VALUE(NAME)                            \
    data += sizeof(temp->NAME)

sizeof(temp.NAME)

Also, I'd suggest putting these on one line, given they are so trivial.

---

toxcore/Messenger.c, line 2680 at r2 (raw file):

#undef COPY_VALUE
#undef COPY_ARRAY

Remove spaces at end of lines. Perhaps set up your editor to not do this.

---

Comments from Reviewable

[robinlinden]

Review status: 1 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

toxcore/Messenger.c, line 2654 at r2 (raw file):

Previously, iphydf wrote…

const struct SAVED_FRIEND temp = { 0 };

Done.

---

toxcore/Messenger.c, line 2657 at r2 (raw file):

Previously, iphydf wrote…

sizeof(temp.NAME)

Also, I'd suggest putting these on one line, given they are so trivial.

Done.

---

toxcore/Messenger.c, line 2680 at r2 (raw file):

Previously, iphydf wrote…

Remove spaces at end of lines. Perhaps set up your editor to not do this.

Done.

---

Comments from Reviewable

[iphydf]

But, since I wrote about half of this, others should sign off as well.

---

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

---

Comments from Reviewable

[JFreegman]

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

---

toxcore/Messenger.c, line 2656 at r3 (raw file):

    const struct SAVED_FRIEND temp = { 0 };

#define COPY_VALUE(NAME) data += sizeof(temp.NAME)

These two defines don't appear to be copying anything. Need better names.

---

toxcore/Messenger.c, line 2802 at r3 (raw file):

static int friends_list_load(Messenger *m, const uint8_t *data, uint32_t length)
{
    if (length % friend_size() != 0) {

You should assign friend_size() to a variable so you don't have to call it numerous times in the same function.

---

Comments from Reviewable

[robinlinden]

Review status: 1 of 2 files reviewed at latest revision, 8 unresolved discussions.

---

toxcore/Messenger.c, line 2656 at r3 (raw file):

Previously, JFreegman wrote…

These two defines don't appear to be copying anything. Need better names.

Done.

---

toxcore/Messenger.c, line 2802 at r3 (raw file):

Previously, JFreegman wrote…

You should assign friend_size() to a variable so you don't have to call it numerous times in the same function.

Done.

---

Comments from Reviewable

[iphydf]

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

---

toxcore/Messenger.c, line 2659 at r4 (raw file):

#define ARRAY_MEMBER(NAME) data += sizeof(temp.NAME)

    // Exactly the same in friend_load, friend_save, and friend_size:

This is no longer true now. Please make it true again.

---

toxcore/Messenger.c, line 2802 at r4 (raw file):

static int friends_list_load(Messenger *m, const uint8_t *data, uint32_t length)
{
    const uint32_t friend_size = friend_size();

You can't reuse the name. Same in haskell: let foo = foo () will fail to compile. (Another reason not to put it in a variable: you have to make up a new name - @JFreegman suggest a name :))

---

Comments from Reviewable

[JFreegman]

---

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

---

Comments from Reviewable

[robinlinden]

Review status: 1 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

toxcore/Messenger.c, line 2659 at r4 (raw file):

Previously, iphydf wrote…

This is no longer true now. Please make it true again.

Done.

---

toxcore/Messenger.c, line 2802 at r4 (raw file):

Previously, iphydf wrote…

You can't reuse the name. Same in haskell: let foo = foo () will fail to compile. (Another reason not to put it in a variable: you have to make up a new name - @JFreegman suggest a name :))

Reverted to using a function.

---

Comments from Reviewable

[nurupo]

This coul be avoided if we memcpy'ed the struct members into a file, instead of memcpy'ing the struct itself. Remember to never memcpy a struct in real code.

@robinlinden why was the tox_one_test.c modified?

---

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

---

Comments from Reviewable

[iphydf]

What could be avoided?

The test was modified to put the network test last. All other tests in tox_one_test are hermetic and don't rely on networking to work.

---

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

---

Comments from Reviewable

[nurupo]

What could be avoided?

The alignment issue this Pr fixes. It exists because toxcore was made to memcpy the struct into the savedata array as it is, instead of memcpy'ing its members. This could be avoided if we memcpy'ed the struct members into the savedata, instead of memcpy'ing the struct itself. Obviously, we can't redo this now, as it would break all existing savedata files and we better use some json/msgpack format if we are going to break it. So the takeaway form this is to remember to never memcpy a struct in real code.

The test was modified to put the network test last. All other tests in tox_one_test are hermetic and don't rely on networking to work.

Ok.

---

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

---

Comments from Reviewable

[GrayHatter]

I didn't verify that this wont eat existing save files.

---

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

---

toxcore/Messenger.c, line 2656 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

> Done.

LGTM

---

Comments from Reviewable