-
Notifications
You must be signed in to change notification settings - Fork 284
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
refactor: Make tox mutex non-recursive. #2652
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2652 +/- ##
==========================================
+ Coverage 73.61% 73.66% +0.05%
==========================================
Files 148 148
Lines 30404 30481 +77
==========================================
+ Hits 22382 22455 +73
- Misses 8022 8026 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess we're missing a test that sets the conference title and a test that actually uses the experimental thread safety option?
2f7f826
to
a18412a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @iphydf)
toxcore/tox.c
line 89 at r1 (raw file):
if (tox->log_callback != nullptr) { tox->log_callback(tox, (Tox_Log_Level)level, file, line, func, message, userdata);
Is locking not needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @nurupo)
toxcore/tox.c
line 89 at r1 (raw file):
Previously, nurupo wrote…
Is locking not needed here?
More accurately, unlocking is not needed here. The log callback happens in really any kind of situation, and client could should never call tox functions inside the log callback.
Previously, iphydf wrote…
Is this documented somewhere? I don't see a mentioning of this where the callback is defined in tox.h. Also, for what purpose does the callback pass |
a18412a
to
5daedb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @nurupo)
toxcore/tox.c
line 89 at r1 (raw file):
Previously, nurupo wrote…
Is this documented somewhere? I don't see a mentioning of this where the callback is defined in tox.h.
Also, for what purpose does the callback pass
Tox *tox
to the user if it's not to be used? (I guess to use of operator== ontox
against some other Tox instance might be a valid use-case?)
Passing Tox to log callbacks was a mistake. Added a comment to the log_callback docs.
Instead, unlock it before entering client callback code.
5daedb8
to
5c93231
Compare
Previously, iphydf wrote…
Do we want to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @nurupo)
toxcore/tox.c
line 89 at r1 (raw file):
Previously, nurupo wrote…
Do we want to remove
Tox *tox
fromtox_log_cb
when we break the API in the future? If so, might be worth noting this somewhere so that we don't forget, e.g. create an issue mentioning it or add a note totox_log_cb
tox.h for ourselves.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 2 of 1 approvals obtained
Instead, unlock it before entering client callback code.
This change is