Skip to content

Avoid use of global mutable state in mono_time on win32.#1135

Merged
iphydf merged 1 commit into
TokTok:masterfrom
iphydf:no-globals-mono-time
Sep 3, 2018
Merged

Avoid use of global mutable state in mono_time on win32.#1135
iphydf merged 1 commit into
TokTok:masterfrom
iphydf:no-globals-mono-time

Conversation

@iphydf
Copy link
Copy Markdown
Member

@iphydf iphydf commented Aug 26, 2018

This uses a trick to get read-write access to this from a const
member function, similar to C++ mutable, but uglier.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 26, 2018
@iphydf iphydf requested a review from zugz August 26, 2018 20:37
@iphydf iphydf force-pushed the no-globals-mono-time branch from bd319e6 to d16a3a7 Compare August 26, 2018 20:38
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 26, 2018

Codecov Report

Merging #1135 into master will decrease coverage by <.1%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1135     +/-   ##
========================================
- Coverage    82.8%   82.8%   -0.1%     
========================================
  Files          82      82             
  Lines       14473   14479      +6     
========================================
+ Hits        11991   11995      +4     
- Misses       2482    2484      +2
Impacted Files Coverage Δ
toxav/video.c 67.4% <100%> (ø) ⬆️
toxcore/mono_time.c 92.8% <100%> (ø) ⬆️
toxcore/mono_time_test.cc 100% <100%> (ø) ⬆️
toxav/audio.c 65.7% <100%> (ø) ⬆️
toxcore/net_crypto.c 93.3% <100%> (-0.1%) ⬇️
auto_tests/dht_test.c 47.1% <0%> (-0.8%) ⬇️
toxcore/friend_connection.c 95.7% <0%> (-0.3%) ⬇️
auto_tests/conference_double_invite_test.c 100% <0%> (ø) ⬆️
auto_tests/lossy_packet_test.c 100% <0%> (ø) ⬆️
... and 8 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 d296490...6872c14. Read the comment docs.

@zugz
Copy link
Copy Markdown

zugz commented Aug 27, 2018

The mutable_this trick is quite ugly, though I don't see that it could cause
any actual problems.

Also it's a shame to have Mono_Time as an argument to the callback, since
user-defined callbacks can't do anything with it (it's an incomplete type).

So how about reverting to having current_time_monotonic_default be the
behaviour of current_time_monotonic when (current_time_callback == nullptr),
rather than being a callback itself.

Then if the const qualifier is dropped from the Mono_Time argument to
current_time_monotonic, the mutable_this field can be dropped.

@iphydf iphydf force-pushed the no-globals-mono-time branch 3 times, most recently from 6fce92a to d306d82 Compare August 30, 2018 22:40
@iphydf
Copy link
Copy Markdown
Member Author

iphydf commented Aug 30, 2018

I've kept the callback argument, so user defined callbacks can call mono_time functions, e.g. to reset the callback to its default, or set it to something else, at the end of the current callback's execution. PTAL.

Copy link
Copy Markdown
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@iphydf iphydf force-pushed the no-globals-mono-time branch 2 times, most recently from b5ac288 to 444e19a Compare September 3, 2018 19:58
This uses a trick to get read-write access to `this` from a `const`
member function, similar to C++ `mutable`, but uglier.
@iphydf iphydf force-pushed the no-globals-mono-time branch from 444e19a to 6872c14 Compare September 3, 2018 20:03
@iphydf iphydf merged commit 6872c14 into TokTok:master Sep 3, 2018
@iphydf iphydf deleted the no-globals-mono-time branch September 3, 2018 20:25
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.8 Oct 7, 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.

4 participants