aksl_snprintf's count out-param should accept NULL #32

Open
opened 2026-08-03 12:49:24 -04:00 by tachikoma · 3 comments
Collaborator

Found by the akbasic port (#26). Raised independently by every one of the ten conversion passes -- the most frequently hit friction in the whole exercise.

```c
akerr_ErrorContext *aksl_snprintf(int *count, char *restrict str, size_t size,
const char *restrict format, ...);
```

`count` is documented Required, so a NULL is a caller error. But the wrapper already turns truncation into `AKERR_OUTOFBOUNDS`, and detecting truncation is the only thing the count was ever used for. Every call site that does not want the byte count -- which is nearly all of them -- has to declare a local purely to throw it away.

The evidence. After the port, akbasic makes 59 `aksl_snprintf` calls. Around twenty of them now carry an

```c
int written = 0;
```

that is written and never read. -Wall -Wextra cannot warn about it, because &written counts as a use, so the dead store is invisible to the compiler and permanent. akbasic's house style puts locals at the top of the function, so several of these sit twenty lines from the call they exist for. src/runtime_struct.c ends up with three aksl_snprintf calls whose &written is garbage interleaved with one raw snprintf whose written is load-bearing, which reads very badly.

What to change. Accept NULL for count and skip the store. This is a relaxation of a preconditionic -- no signature change, no new symbol, no ABI break -- so it can land in 0.2.x. Existing callers passing a real pointer are unaffected.

Test. tests/test_format.c gains a case calling aksl_snprintf(NULL, buf, sizeof(buf), "%s", "x") and asserting success plus the written buffer, and one asserting that a NULL count still gets AKERR_OUTOFBOUNDS on truncation.

Note the asymmetry this resolves. "A NULL out-param is a caller error, not a don't-care" is exactly right for aksl_strlen, where the out-param is the answer and a caller who does not want it has no reason to call. It is wrong for aksl_snprintf, whose answer is the formatted buffer and whose selling point is that the wrapper does the truncation check.

Filed by Tachikoma (Claude Code, Opus 5, 1M context)

Found by the akbasic port (#26). Raised independently by every one of the ten conversion passes -- the most frequently hit friction in the whole exercise. \`\`\`c akerr_ErrorContext *aksl_snprintf(int *count, char *restrict str, size_t size, const char *restrict format, ...); \`\`\` \`count\` is documented Required, so a NULL is a caller error. But the wrapper already turns truncation into \`AKERR_OUTOFBOUNDS\`, and **detecting truncation is the only thing the count was ever used for**. Every call site that does not want the byte count -- which is nearly all of them -- has to declare a local purely to throw it away. **The evidence.** After the port, akbasic makes 59 \`aksl_snprintf\` calls. Around twenty of them now carry an \`\`\`c int written = 0; \`\`\` that is written and never read. `-Wall -Wextra` cannot warn about it, because `&written` counts as a use, so the dead store is invisible to the compiler *and* permanent. akbasic's house style puts locals at the top of the function, so several of these sit twenty lines from the call they exist for. `src/runtime_struct.c` ends up with three `aksl_snprintf` calls whose `&written` is garbage interleaved with one raw `snprintf` whose `written` is load-bearing, which reads very badly. **What to change.** Accept NULL for `count` and skip the store. This is a relaxation of a preconditionic -- no signature change, no new symbol, no ABI break -- so it can land in 0.2.x. Existing callers passing a real pointer are unaffected. **Test.** `tests/test_format.c` gains a case calling `aksl_snprintf(NULL, buf, sizeof(buf), "%s", "x")` and asserting success plus the written buffer, and one asserting that a NULL `count` still gets `AKERR_OUTOFBOUNDS` on truncation. **Note the asymmetry this resolves.** "A NULL out-param is a caller error, not a don't-care" is exactly right for `aksl_strlen`, where the out-param *is* the answer and a caller who does not want it has no reason to call. It is wrong for `aksl_snprintf`, whose answer is the formatted buffer and whose selling point is that the *wrapper* does the truncation check. Filed by Tachikoma (Claude Code, Opus 5, 1M context)
tachikoma added this to the 0.2.x milestone 2026-08-03 12:49:24 -04:00
tachikoma added the api-gapblast-radius:lowstatus::grooming labels 2026-08-03 12:49:24 -04:00
Owner

Honestly we should just remove the parameter entirely then. I'm having a hard time thinking of a reason why we would actually need it.

Just get rid of it.

Honestly we should just remove the parameter entirely then. I'm having a hard time thinking of a reason why we would actually need it. Just get rid of it.
andrew added status::ready and removed status::grooming labels 2026-08-03 14:12:40 -04:00
logikoma was assigned by andrew 2026-08-03 14:12:46 -04:00
Collaborator

Beginning work on this issue. I’ll inspect the current implementation, add focused tests, run the relevant validation, and push the fix.

Beginning work on this issue. I’ll inspect the current implementation, add focused tests, run the relevant validation, and push the fix.
Collaborator

Implemented and pushed as commit 0620370. Removed the count parameter from aksl_snprintf and aksl_vsnprintf, updated API/docs/tests, and retained truncation as AKERR_OUTOFBOUNDS. Validation: build passed and ctest passed 21/21; the configured build has no docs target. Merge request: #42

Implemented and pushed as commit 0620370. Removed the count parameter from aksl_snprintf and aksl_vsnprintf, updated API/docs/tests, and retained truncation as AKERR_OUTOFBOUNDS. Validation: build passed and ctest passed 21/21; the configured build has no docs target. Merge request: https://source.starfort.tech/andrew/libakstdlib/pulls/42
Sign in to join this conversation.