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

Add support for setting a custom monotonic time function in mono_time #1123

Merged
merged 1 commit into from
Aug 26, 2018

Conversation

zugz
Copy link

@zugz zugz commented Aug 25, 2018

(derived from #1095)


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 25, 2018
@codecov
Copy link

codecov bot commented Aug 25, 2018

Codecov Report

Merging #1123 into master will increase coverage by 0.1%.
The diff coverage is 92%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1123     +/-   ##
========================================
+ Coverage    82.5%   82.6%   +0.1%     
========================================
  Files          81      81             
  Lines       14414   14437     +23     
========================================
+ Hits        11895   11929     +34     
+ Misses       2519    2508     -11
Impacted Files Coverage Δ
toxcore/mono_time_test.cc 100% <100%> (ø) ⬆️
toxcore/mono_time.c 92.8% <81.8%> (-7.2%) ⬇️
toxcore/LAN_discovery.c 82% <0%> (-2.9%) ⬇️
toxcore/onion_client.c 94.2% <0%> (-0.8%) ⬇️
toxcore/network.c 83.6% <0%> (+0.2%) ⬆️
toxav/toxav.c 68.6% <0%> (+0.4%) ⬆️
toxcore/DHT.c 77.5% <0%> (+0.8%) ⬆️
toxav/msi.c 66.7% <0%> (+2.2%) ⬆️

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 acc19a2...01e2cc5. Read the comment docs.

Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

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


toxcore/mono_time.h, line 62 at r1 (raw file):

 * to increase monotonically.
 */
void set_time_monotonic_callback(Mono_Time *mono_time,

Let's name this something starting with "mono_time", so this file is more api-standard compliant. We try to place functions belonging to a module in the same "namespace". (Same for the cb above)


toxcore/mono_time.c, line 78 at r1 (raw file):

{
    if (time_monotonic_callback == nullptr) {
        time_monotonic_callback = current_time_monotonic_default;

nit: assigning a parameter is not great. Assignment is generally icky, but parameter assignment is often considered bad style (in JavaScript, it's a really bad idea, in other languages it's just a little frowned upon). Let's put if (..) { mono_time->tmc = ..; } else { mono_time->tmc = ctmd; } instead.

@iphydf iphydf changed the title Allow custom mono_time function Add support for tests to set a custom monotonic time function in mono_time Aug 25, 2018
@iphydf iphydf changed the title Add support for tests to set a custom monotonic time function in mono_time Add support for setting a custom monotonic time function in mono_time Aug 25, 2018
@iphydf
Copy link
Member

iphydf commented Aug 25, 2018

@zugz can you add a test for this functionality to mono_time_test.cc?

Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)


toxcore/mono_time.c, line 47 at r2 (raw file):

    mono_time->time = 0;
    mono_time->current_time_callback = current_time_monotonic_default;

Move this one line down, so initialisation of the fields happens in declaration order and so the callback-related fields are initialised together.


toxcore/mono_time.c, line 85 at r2 (raw file):

    }

    mono_time->user_data = user_data;

Move this to the second branch, and add an assignment of nullptr to the first branch. This way we avoid pointing at user data when nothing will ever be able to read it.


toxcore/mono_time_test.cc, line 48 at r2 (raw file):

  uint64_t test_time = 42137;

  mono_time_set_current_time_callback(mono_time, test_current_time_callback, &test_time);

test_current_time_callback is probably better as a lambda: [](void *user_data) { return *(uint64_t *)user_data; }.

@zugz
Copy link
Author

zugz commented Aug 26, 2018 via email

Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: 1 change requests, 0 of 1 approvals obtained


toxcore/mono_time.c, line 47 at r2 (raw file):

Previously, iphydf wrote…

Move this one line down, so initialisation of the fields happens in declaration order and so the callback-related fields are initialised together.

Ok, can you move the callback initialisation code up then?

Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @zugz)


toxcore/mono_time.c, line 47 at r2 (raw file):

Previously, iphydf wrote…

Ok, can you move the callback initialisation code up then?

Unresolved.

Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 2 of 2 files at r4.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @zugz)

@zugz zugz force-pushed the customClockReduct branch from 6566fb8 to 01e2cc5 Compare August 26, 2018 20:07
@zugz
Copy link
Author

zugz commented Aug 26, 2018 via email

@iphydf iphydf merged commit 01e2cc5 into TokTok:master Aug 26, 2018
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.7 Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants