cleanup: Move all vptr-to-ptr casts to the beginning of a function. #2512
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2512 +/- ##
==========================================
- Coverage 69.03% 69.01% -0.03%
==========================================
Files 89 89
Lines 27774 27781 +7
==========================================
- Hits 19175 19174 -1
- Misses 8599 8607 +8 ☔ View full report in Codecov by Sentry. |
iphydf
force-pushed
the
void-cast
branch
9 times, most recently
from
761da44 to
4cd7463
Compare
iphydf
force-pushed
the
void-cast
branch
2 times, most recently
from
861e916 to
5b869c7
Compare
These casts are effectively part of the function type, so it makes sense to have them at the beginning.
There was a problem hiding this comment.
A bit conflicted about this change as I'm more of "declare it right before you use it" mindset. Most of these functions start with some sort of argument check, and it feels weird to declare the type cast before the check and then not use it in the check at all. In a rare case when the check actually needs to know the type of the void * argument, then sure, in this case it's fine to declare beforehand.
Your "it's part of the function type" reasoning makes sense, but it's a bit jarring to read a declaration and then proceed reading the check and not see the declared thing being used in it, it kind of annoys me, like what was the point of me even reading it and keeping it in mind?
Reviewed 10 of 11 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 0 of 1 approvals obtained
There was a problem hiding this comment.
Reviewed 10 of 11 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)
toxav/bwcontroller.c line 211 at r2 (raw file):
static int bwc_handle_data(Messenger *m, uint32_t friendnumber, const uint8_t *data, uint16_t length, void *object) { BWController *bwc = (BWController *)object;
Should we check if it's null like we do with the others? on_update() uses it without a null check. There are a few other missing null checks, you can decide if they're necessary or not.
There was a problem hiding this comment.
Anyway, LGTM.
(Not directly approving to avoid the auto-merge as JFreegman wants you to see his message).
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @zoff99)
toxav/bwcontroller.c line 211 at r2 (raw file):
Previously, JFreegman wrote…
Should we check if it's null like we do with the others?
on_update()uses it without a null check. There are a few other missing null checks, you can decide if they're necessary or not.
I'm hesitant to touch toxav more than necessary to pass tokstyle. If I touch it too much, @zoff99's toxav PR will be harder and harder to merge.
These casts are effectively part of the function type, so it makes sense to have them at the beginning.
This change is