-
Notifications
You must be signed in to change notification settings - Fork 998
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
Avoid casts that don't change the pointer type #6785
Conversation
If the returned pointer type ever changes, the absence of the cast will give us a compiler warning due to the value being implicitly cast to an incompatible pointer type. An explicit cast, on the other hand, tells the compiler to silence the warning and do as written.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6785 +/- ##
=======================================
Coverage 98.60% 98.60%
=======================================
Files 79 79
Lines 14642 14642
=======================================
Hits 14438 14438
Misses 204 204 ☔ View full report in Codecov by Sentry. |
Do you think it makes sense to run your diagnostic program as part of every PR (under code-quality/lint-c one presumes)? Or perhaps just occasionally (we have R-CMD-check-occasional which is not currently in a working state, or just .dev/CRAN_release for manual checks)? |
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.
Great finds!
Feel free to merge before/after adding a CI check; if you'd rather punt on the CI check, please file a follow-up.
For mistakes that are easy to fix automatically, Coccinelle patches may be written. If a patch finds a mistake, the linter will ask the human to fix either the code or the patch.
de686d1
to
54a0c14
Compare
Coccinelle installation adds 11 seconds to the On a broader note, in addition to pre-written data.table-specific linters, might we benefit from more generic static checking for the C code? A |
In C, explicit pointer casts are mostly needed to work with the byte representation of a larger type. In other cases, either the implicit cast (e.g. to and from
void*
) is already fine and won't cause any warnings, or the code is doing something suspicious (e.g.Rboolean*
↔int*
) and we'd rather have the warning from the implicit cast.For example, adding an explicit cast from
LOGICAL(...)
toint*
doesn't prevent a warning, becauseLOGICAL(...)
already returns anint*
. If the pointer type returned byLOGICAL(...)
ever changes, the absence of the cast will cause the compiler to issue a diagnostic: then it would be an implicit cast to a different pointer type, likely a violation of the "strict aliasing" rule, something bad to warn about. An explicit cast, on the other hand, tells the compiler to silence the warning and do as written.Initially found while working on #6782. More examples located using the following Coccinelle script:
and the following command line:
The remaining examples found by
spatch
are all insnprintf(...)
calls casting non-pointer values like anuint64_t
corresponding to thePRIu64
conversion specifier. It's probably fine to keep the code defensive and leave these casts in. If the non-pointer type does change, the cast should prevent most problems, unless the cast causes a signed overflow (to be caught by UBSan).