Make saving and loading the responsibility of Tox rather than Messenger by zugz · Pull Request #1213 · TokTok/c-toxcore

zugz · 2018-09-27T19:04:50Z

This change is

iphydf · 2018-09-28T04:49:07Z

Perhaps while you're changing this code already, remove the vla and use malloc/free?

iphydf · 2018-09-28T04:51:57Z

/** * Load a section. * * @...

codecov · 2018-09-28T17:16:58Z

Codecov Report

Merging #1213 into master will decrease coverage by <.1%.
The diff coverage is 92.8%.

@@           Coverage Diff            @@
##           master   #1213     +/-   ##
========================================
- Coverage    83.1%     83%   -0.1%     
========================================
  Files          82      82             
  Lines       14694   14683     -11     
========================================
- Hits        12218   12196     -22     
- Misses       2476    2487     +11

Impacted Files	Coverage Δ
auto_tests/messenger_test.c	`85.8% <ø> (-1.5%)`	⬇️
toxcore/group.c	`76.2% <100%> (+0.1%)`	⬆️
toxcore/DHT.c	`76.8% <100%> (ø)`	⬆️
toxcore/Messenger.c	`86.8% <100%> (-0.1%)`	⬇️
toxcore/state.c	`56.4% <50%> (-4.8%)`	⬇️
toxcore/tox.c	`66.7% <93.9%> (+1%)`	⬆️
toxav/msi.c	`64.8% <0%> (-0.9%)`	⬇️
toxcore/onion_client.c	`95.3% <0%> (-0.7%)`	⬇️
toxav/toxav.c	`68.1% <0%> (-0.5%)`	⬇️
toxcore/friend_connection.c	`93.1% <0%> (-0.3%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5c782...744dc2f. Read the comment docs.

zugz · 2018-09-29T06:44:39Z

I factored the save invariance and associated tests out as a separate PR, #1215.

iphydf · 2018-10-07T23:01:52Z

Replace !num with num == 0 when num is an integer, not a bool. Also, would it make sense to return -1 on the inverted condition, considering it an error return, and returning the success/failure of state_load at the end of the function?

iphydf · 2018-10-07T23:03:12Z

Why did you rename savedata to data? Now the .c and .h files are out of sync. If you want to use the name "data" in the function, you can assign savedata to data (which may not be such a bad idea anyway, since it avoids assigning to parameters).

iphydf

I need to leave a comment, I can't just put comments on the code without also putting a comment in this box, so here is a comment. Thanks Github.

iphydf · 2018-10-16T22:23:47Z

Can you rebase or merge upstream/master?

zugz · 2018-10-17T05:38:03Z

* Tuesday, 2018-10-16 at 15:23 -0700 - iphydf <notifications@github.com>:

Can you rebase or merge upstream/master?

Done

iphydf

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)

auto_tests/messenger_test.c, line 268 at r3 (raw file):

END_TEST

/* This test is disabled, because saving is now handled by Tox rather than Messenger. */

Is there value in keeping this test around but commented out? Are you planning to delete or revive it in the future?

testing/Messenger_test.c, line 137 at r3 (raw file):

    }

    /* Loading disabled, because loading is now handled by Tox rather than Messenger */

Should this also be deleted instead of commented out?

toxcore/group.h, line 457 at r1 (raw file):

Previously, iphydf wrote…

/**
 * Load a section.
 *
 * @...

Done. (comment on behalf of zugz)

toxcore/state.h, line 25 at r3 (raw file):

#define TOP_STATE_COOKIE_TYPE  0x01ce

typedef enum Top_State_Type {

State_Type, otherwise it's not in the module namespace of module "state", but instead in the namespace of module "top", which we don't have.

Separately, this is not a great place for this enum to live, but I don't have a better suggestion, so let's keep it here for now.

toxcore/state.c, line 109 at r1 (raw file):

Previously, zugz (zugz) wrote…

Hmm... I wouldn't know how best to do it, and I understand we intend to redo saving completely eventually. So I'd rather defer.

Ok, the way you do it is by serialising the integer instead of memcpying it into a byte array. I'm fine with deferring.

toxcore/tox.c, line 368 at r2 (raw file):

Previously, iphydf wrote…

Replace !num with num == 0 when num is an integer, not a bool. Also, would it make sense to return -1 on the inverted condition, considering it an error return, and returning the success/failure of state_load at the end of the function?

Done (comment on behalf of zugz).

toxcore/tox.c, line 604 at r2 (raw file):

Previously, iphydf wrote…

Why did you rename savedata to data? Now the .c and .h files are out of sync. If you want to use the name "data" in the function, you can assign savedata to data (which may not be such a bad idea anyway, since it avoids assigning to parameters).

Done (comment on behalf of zugz).

auto_tests/save_load_test.c, line 100 at r1 (raw file):

Previously, iphydf wrote…

Perhaps while you're changing this code already, remove the vla and use malloc/free?

Done (in separate PR).

zugz · 2018-10-17T21:34:26Z

* Wednesday, 2018-10-17 at 05:17 -0700 - iphydf <notifications@github.com>:

*[testing/Messenger_test.c, line 137 at r3](https://reviewable.io/reviews/toktok/c-toxcore/1213#-LP0w4_o8qCfdpnPxHUy:-LP0w4_o8qCfdpnPxHUz:bqszos9) ([raw file](https://github.com/toktok/c-toxcore/blob/7f90dca13d91a2e47f859f31a29dc12056e7f40a/testing/Messenger_test.c#L137)):* > ```C > } > > /* Loading disabled, because loading is now handled by Tox rather than Messenger */ > ``` Should this also be deleted instead of commented out?

I'd rather leave it, in case someone one day wants to adapt it. Done the rest. I can fire up reviewables to say so there if that would really be helpful. I looked again, and the loading code in Messenger_test is trivial enough that actually I don't think there's much point keeping it around. I've deleted it now.

iphydf

Reviewable status: 1 change requests, 0 of 1 approvals obtained

auto_tests/messenger_test.c, line 268 at r3 (raw file):

Previously, iphydf wrote…

Is there value in keeping this test around but commented out? Are you planning to delete or revive it in the future?

Done (comment on behalf of zugz).

testing/Messenger_test.c, line 137 at r3 (raw file):

Previously, zugz (zugz) wrote…

Wednesday, 2018-10-17 at 05:17 -0700 - iphydf notifications@github.com:

testing/Messenger_test.c, line 137 at r3 (raw file): > C > } > > /* Loading disabled, because loading is now handled by Tox rather than Messenger */ >

Should this also be deleted instead of commented out?

I'd rather leave it, in case someone one day wants to adapt it.

Done the rest. I can fire up reviewables to say so there if that would really be helpful. I looked again, and the loading code in Messenger_test is trivial enough that actually I don't think there's much point keeping it around. I've deleted it now.

Done (comment on behalf of zugz).

toxcore/state.h, line 25 at r3 (raw file):

Previously, iphydf wrote…

State_Type, otherwise it's not in the module namespace of module "state", but instead in the namespace of module "top", which we don't have.

Separately, this is not a great place for this enum to live, but I don't have a better suggestion, so let's keep it here for now.

Done (comment on behalf of zugz).

zugz force-pushed the saveInTox branch from f61ec41 to 890870d Compare September 27, 2018 19:46

zugz added this to the v0.2.x milestone Sep 27, 2018

iphydf requested changes Sep 28, 2018

View reviewed changes

zugz force-pushed the saveInTox branch 4 times, most recently from f1c0e13 to 9038ec4 Compare September 28, 2018 16:53

zugz force-pushed the saveInTox branch 2 times, most recently from b50824f to 4d084e7 Compare September 29, 2018 06:41

zugz mentioned this pull request Sep 30, 2018

Fix memory leak in tcp server by wiping priority queues on deletion. #1216

Merged

iphydf reviewed Oct 7, 2018

View reviewed changes

iphydf requested changes Oct 7, 2018

View reviewed changes

zugz force-pushed the saveInTox branch from 4d084e7 to f86bc31 Compare October 10, 2018 19:11

zugz force-pushed the saveInTox branch from f86bc31 to 7f90dca Compare October 17, 2018 05:37

iphydf requested changes Oct 17, 2018

View reviewed changes

iphydf approved these changes Oct 19, 2018

View reviewed changes

Make saving and loading the responsibility of Tox rather than Messenger

744dc2f

zugz force-pushed the saveInTox branch from ae6ab93 to 744dc2f Compare October 20, 2018 09:08

zugz merged commit 744dc2f into TokTok:master Oct 20, 2018

zugz deleted the saveInTox branch October 20, 2018 11:40

robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make saving and loading the responsibility of Tox rather than Messenger#1213

Make saving and loading the responsibility of Tox rather than Messenger#1213
zugz merged 1 commit into
TokTok:masterfrom
zugz:saveInTox

zugz commented Sep 27, 2018 •

edited by iphydf

Loading

Uh oh!

iphydf Sep 28, 2018

Uh oh!

iphydf Sep 28, 2018

Uh oh!

Uh oh!

codecov Bot commented Sep 28, 2018 •

edited

Loading

Uh oh!

zugz commented Sep 29, 2018 via email

Uh oh!

iphydf Oct 7, 2018

Uh oh!

iphydf Oct 7, 2018 •

edited

Loading

Uh oh!

iphydf left a comment

Uh oh!

iphydf commented Oct 16, 2018

Uh oh!

zugz commented Oct 17, 2018 via email

Uh oh!

iphydf left a comment

Uh oh!

zugz commented Oct 17, 2018 via email •

edited

Loading

Uh oh!

iphydf left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

zugz commented Sep 27, 2018 • edited by iphydf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iphydf Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

iphydf Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zugz commented Sep 29, 2018 via email

Uh oh!

iphydf Oct 7, 2018

Choose a reason for hiding this comment

Uh oh!

iphydf Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iphydf left a comment

Choose a reason for hiding this comment

Uh oh!

iphydf commented Oct 16, 2018

Uh oh!

zugz commented Oct 17, 2018 via email

Uh oh!

iphydf left a comment

Choose a reason for hiding this comment

Uh oh!

zugz commented Oct 17, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iphydf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

zugz commented Sep 27, 2018 •

edited by iphydf

Loading

codecov Bot commented Sep 28, 2018 •

edited

Loading

iphydf Oct 7, 2018 •

edited

Loading

zugz commented Oct 17, 2018 via email •

edited

Loading