-
Notifications
You must be signed in to change notification settings - Fork 291
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
cleanup: Fix a few more clang-tidy warnings. #2406
Conversation
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 98 of 98 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)
toxcore/DHT.c
line 781 at r1 (raw file):
const IPPTsPng *ipptp = NULL; if (net_family_is_ipv4(sa_family) || client->assoc4.timestamp >= client->assoc6.timestamp) {
Is this an intentional logic change? Previously, we set ipptp = &client->assoc6;
after confirming that sa_family is ipv6, even if client->assoc4.timestamp >= client->assoc6.timestamp
. Now we set ipptp = &client->assoc4;
if client->assoc4.timestamp >= client->assoc6.timestamp
even if sa_family is ipv6.
toxcore/tox.c
line 816 at r1 (raw file):
if (tox->m == nullptr) { switch (m_error) { case MESSENGER_ERROR_PORT:
Intentional fallthrough? Should probably comment that
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: 0 of 1 approvals obtained (waiting on @JFreegman)
toxcore/DHT.c
line 781 at r1 (raw file):
Previously, JFreegman wrote…
Is this an intentional logic change? Previously, we set
ipptp = &client->assoc6;
after confirming that sa_family is ipv6, even ifclient->assoc4.timestamp >= client->assoc6.timestamp
. Now we setipptp = &client->assoc4;
ifclient->assoc4.timestamp >= client->assoc6.timestamp
even if sa_family is ipv6.
Good point. Undone.
toxcore/tox.c
line 816 at r1 (raw file):
Previously, JFreegman wrote…
Intentional fallthrough? Should probably comment that
I usually consider "fallthrough" to be a case that does work and then doesn't break (which we don't allow). This fallthrough is effectively an "or" case. Should we document such cases (e.g. group.c:2146)?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2406 +/- ##
==========================================
+ Coverage 74.49% 74.66% +0.17%
==========================================
Files 87 87
Lines 26194 26190 -4
==========================================
+ Hits 19512 19555 +43
+ Misses 6682 6635 -47
☔ View full report in Codecov by Sentry. |
Maybe I'm misremembering because I thought we already did that. It's probably not necessary. |
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 96 of 96 files at r2, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained
This change is