BUG: Parenthesize macro arguments and replacement lists - #61
hjmjohnson merged 1 commit into
Conversation
8caae1c to
74d6fff
Compare
74d6fff to
abaac8b
Compare
abaac8b to
5952e1b
Compare
|
Rebased onto I checked every call site for an observable behavior change. There is none — this is prophylactic. Evidence below. The PR title says "Parenthesise"; the commit subject is now The call-site checkAn unparenthesized argument only misbehaves when a caller passes something whose precedence differs from a primary expression. Extracted every argument of every Everything else is a plain identifier. The only non-identifier is
So there is no red to prove. Class is BUILD: 157 clang-tidy Installed-header check, re-runVerified with
The two tool headers are not in any target's Backslash continuations realignedInserting the parentheses pushed the line text past the continuation backslashes, leaving every touched macro block ragged. Each changed line now carries its backslash back at the column it had on Message cleanupRemoved the Replaced the bare Corrected one claim: the body said Build and test
|
hjmjohnson
changed the title
5952e1b to
857e6cb
Compare
Clears all 155 bugprone-macro-parentheses findings: 152 in nifti2/nifti_tool.c, 2 in niftilib/nifti1_tool.c, 1 in fsliolib/fslio.c. An unparenthesized argument takes the precedence of whatever the caller passed. NT_FILL(..., rv) expands to `rv = ...` and NT_MAT33_TO_MAT44 to `m44.m[0][0] = ...`, which break for an argument that is not already a postfix expression. Every current caller passes a plain identifier, a literal or `nim->sto_xyz`, so nothing observable moves. The NT_DCONVERT dtype and stype parameters stay bare behind a NOLINT fence: they name types, so `(dtype) * pd = dptr;` would be a cast expression rather than a declaration. FSL_RADIOLOGICAL is in an installed header, so its replacement list changes from -1 to (-1); its value does not.
857e6cb to
fffc091
Compare
|
Rebased onto current The conflict, and why the resolution takes both sides
(failure) = 0; \
for(index=0; index<(nvals); index++) { \Measured with clang-tidy, per file
This corrects the numbers in the original commit message, which said 170 findings Two things fixed on top of the rebaseBackslash alignment had regressed. Parenthesizing lengthened the
Each is realigned one space past its own longest line, so the whole block shifts
Verification
Churn is 63 added / 59 deleted; Worth a follow-up, out of scope here: clang-tidy is configured but not run in CI, |
dbfb59d
into
InsightSoftwareConsortium:master
2 participants
170 bugprone-macro-parentheses findings, of which this clears 157.
An unparenthesised macro argument silently takes the precedence of
whatever the caller passed. NT_FILL(..., rv) expanding to
rv = ...does the wrong thing for any caller passing an expression, and
NT_MAT33_TO_MAT44(m33, m44) expanding to
m44.m[0][0] = ...breaks forany argument that is not a plain identifier. No current call site trips
on it, which is exactly why it would otherwise be found the hard way.
Sites: the NT_FILL, NT_DCONVERT and NT_MAT* macro families in
nifti_tool.h and nifti1_tool.h, and FSL_RADIOLOGICAL in fslio.h.
Two notes on what was not taken from the automated fix:
clang-tidy parenthesised the
dtypeandstypeparameters ofNT_DCONVERT_NO_CHECKS and NT_DCONVERT_W_CHECKS. Those are type
names, so
(dtype) * pd = dptr;is a cast expression rather than adeclaration, and the build fails outright. Those four are reverted
and the two macros carry a NOLINT fence saying why the check cannot
be satisfied there.
FSL_RADIOLOGICAL is in an installed header, so its text does change:
-1 becomes (-1). Its value does not. Verified by compiling against
the installed fslio.h and printing it, and by diffing gcc -dM -E over
every installed header: that macro's replacement list is the only
definition that differs from master.
Thirteen findings are deliberately left, all on lines the open PRs
already rewrite: the four NT_DT_* constants in each tool header and the
two NT_DCONVERT loop bounds (#22), the QSTR macro in both io files
(#22 and #24), and three in nifti_tester001.c (#22). #22 fixes this
same defect class by hand in those places; this is the remainder, and
the two do not touch a line in common.
Interface impact: none. On the union of all these changes, configured with
USE_FSL_CODE=ONandUSE_CIFTI_CODE=ON: all 448 exported symbols acrosslibniftiio,libnifti2,libznz,libfslio,libnifticdfandlibciftiare identical tomasterundernm -D --defined-only, and all ten installed headers are identical undergcc -E -P. Undergcc -dM -Eone macro definition differs, intentionally and only in text: #61 makesFSL_RADIOLOGICALread(-1)so it is safe inside an expression. Its value is still-1, checked by compiling against each installedfslio.hand printing it.Verification. This branch: builds with gcc 16.1.1,
ctestunchanged frommaster(2 of 345 fail onmasteritself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8,ctest345/345 under each, and the whole suite under valgrind memcheck with--trace-children=yesgives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9
-Wsign-conversion(5 infslio.cfor #22, 2 innifti2_io.cand 2 innifti_tester001.cfor #24) and 8-Wcalloc-transposed-argsinnifti_findhdrnameandnifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.One of a set of independent, single-purpose PRs. Each bases on
masterand can be merged on its own, in any order.The full set of PRs (35)
The union of all of them is on the fork as
all-changes, if you want to build and test the lot at once.CI and build
Configuration and documentation
Defects
Warning and check classes