ENH: Pass calloc its arguments in the documented order - #130
Open
hjmjohnson wants to merge 3 commits into
Open
hjmjohnson wants to merge 3 commits into
hjmjohnson wants to merge 3 commits into
Conversation
A macro whose body is a bare { ... } block cannot be used as a
statement:
if( cond )
FSLIOERR(...); /* the ; ends the if, the block runs */
else /* ...and this is a syntax error */
No current call site is written that way -- FSLIOERR has no `else`
after it anywhere, and none of the nifti_*_test macros appears in an
if or else arm -- so the change is prophylactic and nothing observable
moves. The next person to write one, though, gets a syntax error a
long way from the cause. clang reports the call sites as
-Wextra-semi-stmt, because the trailing semicolon at each is an empty
statement.
Wrapped: FSLIOERR in fslio.c and the seven nifti_*_test macros in
nifti_tester001.c. The NT_FILL and NT_MAT* families in the tool
headers already have do/while bodies.
Two genuine empty statements go too: a doubled semicolon inside
unescape_string() in both io files, and a stray semicolon after the
closing brace of an if block in FslClose().
FSLIOERR is worth calling out: it expands to fprintf plus
exit(EXIT_FAILURE), and of its roughly fifty call sites several are
the whole body of a braceless if. Those are correct today only
because the macro always exits.
(cherry picked from commit 4ac84e5)
The backslashes sat at four different columns within a single macro, one of them past 80. Each macro is now aligned to its own width, one space past its longest line, so a block stays as narrow as its content allows rather than being widened by one long line. Only the seven macros the preceding commit gives a do/while(0) body are touched. (cherry picked from commit 3f14c7e)
Four calls were written calloc(sizeof(char), n) rather than calloc(n, sizeof(char)), which gcc 14 and later diagnose as -Wcalloc-transposed-args. There is no behavior change: calloc multiplies its two arguments and sizeof(char) is 1 by definition, so the same number of zeroed bytes is returned either way and the overflow check is the same product. The warning is worth clearing anyway, because the diagnostic exists to catch the case where the element size is not 1, at which point the transposition produces a buffer of the wrong size. Leaving benign instances in the tree trains the reader to ignore the warning that will one day be real. Ten further instances are left alone deliberately. They are in nifti_findhdrname(), nifti_findimgname(), nifti_makehdrname() and nifti_makeimgname() in both io files, all of which an open pull request rewrites while keeping the transposed order. They will need fixing on top of that work; changing them here would only conflict with it. (cherry picked from commit affba1e)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-submission of #50, reverted from
masteron 2026-09-24. Content isunchanged from the original.
Position 8 of 11 in the deep stack. Base:
stack/fix-cifti-null-stream.Based on the pull request above it in the stack, so the diff shown here is
this change alone. Merge the stack bottom-up.
Stack order
stack/test-regression-coveragemasterstack/pr-fix-alloc-null-checksstack/test-regression-coveragestack/fix-axml-skip-depthstack/pr-fix-alloc-null-checksstack/pr-fix-analyzer-leaksstack/fix-axml-skip-depthstack/pr-fix-sign-conversionstack/pr-fix-analyzer-leaksstack/fix-fslio-64bit-arithmeticstack/pr-fix-sign-conversionstack/fix-cifti-null-streamstack/fix-fslio-64bit-arithmeticstack/pr-fix-calloc-transposed-args<- this PRstack/fix-cifti-null-streamstack/pr-fix-shorten-64-to-32stack/pr-fix-calloc-transposed-argsstack/fix-image-read-complex-checkstack/pr-fix-shorten-64-to-32stack/pr-fix-xml-read-errorsstack/fix-image-read-complex-checkThe order is the order these changes sat on
masterbefore the revert, soit builds and tests at every step.
Commits introduced by this PR
Ordering for all the re-submitted work is tracked in #84.