Skip to content

Fix what CLion's inspections found, and gate the two checks worth keeping - #77

Merged
theAndreas merged 3 commits into
masterfrom
clion-fixes
Sep 14, 2026
Merged

Fix what CLion's inspections found, and gate the two checks worth keeping#77
theAndreas merged 3 commits into
masterfrom
clion-fixes

Conversation

@theAndreas

@theAndreas theAndreas commented Sep 11, 2026 •
edited by AndreasBur
Loading

Copy link
Copy Markdown
Collaborator

CLion's inspections were run over the tree. Four of the findings were bugs; the rest split
into a rule worth enforcing and a list of things that are right as they are.

The bugs

  • Bit::readBitsValue forwarded to the two-argument overload and lost the mask, so it read
    the whole field.
  • MsgCmdParser::getCommand used atoi, whose int is 16 bits on the AVR: "65537"
    wrapped to 1 and was answered as a valid command. The comment there claimed strtol
    would only reach "the same answer over more code" - true for a non-number, wrong for an
    overflow.
  • getParameter returned the last character when a message had no delimiter, so "12"
    handed the parameter parser a "2"; for an empty message it indexed length() - 1, a
    strlen of zero minus one.
  • DisplayColor::dimmColor multiplied two bytes, which promote to signed 16-bit int on
    the AVR - 255 * 255 does not fit.

~AnimationSnake was also declared and never defined.

Why a digit loop and not strtol

strtol reports the overflow and was the first fix, but it links a routine that parses
bases, signs and a long where a command number is one of seventeen:

AVR flash
master (atoi, wraps) 48 078
strtol 48 704 (+626)
digit loop, refusing as it counts (this branch) 48 104 (+26)

Same rejections either way - the boundary tests in serial_test.cpp pass on both.

The rule, and where it stops

readability-convert-member-functions-to-static and
readability-make-member-function-const are on in .clang-tidy, so this stays answered
per pull request instead of once. The sweep itself costs no flash.

It stops at platform/simulator/. clang-tidy only runs over -DPLATFORM=simulator, so it
sees one backend of four; following it there would have made Storage::read, System,
WebHost and WebTransport static while avr-dx, esp32 and rp2350 hold the same methods
byte for byte as const. A stand-in whose API differs from what it stands in for has lost
the point of being one. They keep suppressions naming the check and the reason.

What is not kept

The inspection snapshot. A few thousand findings carrying line numbers, stale by the next
commit and enforced by nothing - the failure CLAUDE.md's sizes-in-prose rule exists to
prevent, at 1.5 MB. docs/code-inspections.md keeps what does not rot: the disabled
inspections and why, the findings read and refused, and what the new tests cover.
.idea/editor.xml stays, since it is where those three inspections are actually switched
off.

Validation

Run locally, all green: simulator build + WordclockTests; AVR, ESP32 and RP2350 firmware
builds; ESP32 and RP2350 host runners; clang-tidy and cppcheck with the branch's own
configuration, zero findings; documented-sizes.py for all three targets. Both
intermediate commits build and test on their own. Hardware runtime behaviour was not
exercised.

🤖 Generated with Claude Code

AndreasBurnickl and others added 2 commits September 13, 2026 17:12
@AndreasBurnickl @claude
Fix the four bugs the inspections turned up, and pin each with a test
ac07680 
Bit::readBitsValue forwarded to the two-argument overload and dropped the
mask on the way, so it read the whole field wherever it was called with one.

MsgCmdParser::getCommand ran the message through atoi, which returns an int:
on the AVR that is 16 bits, so "65537" wrapped to 1 and was answered as a
valid command. The comment that stood there argued strtol would only reach
"the same answer over more code" - true for a message that is not a number,
wrong for one that overflows. It now counts digits up itself and refuses the
moment the running number passes the last command. strtol would do the same
and costs 566 bytes of flash for a routine that parses bases, signs and a
long; the whole branch is worth 26 bytes with the loop and 626 with strtol.

getParameter returned the last character of a message that had no delimiter,
so "12" handed the parameter parser a "2" to read options out of - and for an
empty message it asked for index length() - 1, a strlen of zero minus one and
not a position at all. It is the terminator now, which is where the empty
parameter lives. The parser is reachable directly, not only behind
Communication's empty-message filter, so the empty case is a real one.

DisplayColor::dimmColor multiplied two bytes, which promote to signed 16-bit
int on the AVR: 255 * 255 does not fit and the product was undefined. One
operand is widened first, the way Pixel::dimmColor already did it.

~AnimationSnake was declared and never defined - a link error waiting for the
first caller that destroyed one, which the long-lived animation pool never
does.

serial_test covers the empty message, non-numbers, out-of-range and the
overflowing numbers up to 49 digits, plus leading zeroes and the highest
command. The colour test walks every one of the 65536 byte pairs, though only
the target can show the overflow: the host's int is wider. The snake test
gives one automatic storage so the destructor runs at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AndreasBurnickl @claude
Make static or const what needs no instance state, and gate it
c980df3 
readability-convert-member-functions-to-static and
readability-make-member-function-const are on in .clang-tidy, so this stays
answered on every pull request rather than once. Style is still not
clang-tidy's to have an opinion about - these two are not about style but
about what a function reaches, which is the same question the singletons in
this tree keep raising: a helper that only maps an index to a column has no
business taking a this pointer.

The wxWidgets event handlers in PixelsFrame keep theirs, because the event
tables want member-function pointers, and their suppressions name only that
check. WordclockApp::OnInit says override instead of repeating virtual.

Transformation loses its include of Display.h to the .cpp, where the shift
functions actually touch the display; the header only ever needed the type
for its own declarations. NightSwitch's test for a fitted power switch is an
if constexpr, since Power::isSwitchFitted() answers at compile time and the
branch that is not taken has no reason to be emitted.

Costs 26 bytes of flash on the AVR against master, all of it in the command
parser above; the sweep itself is free.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AndreasBur AndreasBur changed the title Clion fixes Fix what CLion's inspections found, and gate the two checks worth keeping Sep 13, 2026
@AndreasBurnickl @claude
Let the simulator's stand-ins keep the shape of what they stand in for
d32d8a9 
clang-tidy only ever runs over -DPLATFORM=simulator, so the two checks the
commit before this turned on see one backend out of four. Followed literally
they would have made Storage::read, the whole of System, WebHost and
WebTransport static in platform/simulator while avr-dx, esp32 and rp2350 hold
the same methods byte for byte as const - and a stand-in whose API differs
from the thing it stands in for has lost the one property it exists for. The
avr-dx System::isConsoleProtected is the same one-line "return false" and was
never touched, only because nothing analyses it.

So they stay non-static, with the reason at the suppression rather than in a
commit nobody will find again. BH1750, Pixels, Storage and System name the
check; PixelsFrame's wx handlers keep their own, which are about the event
tables and not about this.

One file of the CLion project comes with it. .idea/editor.xml is where the three
inspections this project does not want are switched off, and the only record of
which three and why - docs/code-inspections.md would otherwise name a file that
is not there. The rest of that directory is derivable from CMakeLists.txt and
rewritten whenever the IDE feels like it, so the root .gitignore keeps it out
and excepts that one file back in. That is also where .idea/.gitignore's job
went: without it CLion's workspace.xml would start showing up as untracked.

What is deliberately not kept is the inspection snapshot: a few thousand
findings carrying line numbers, stale by the next commit, and unenforceable -
the same failure the sizes-in-prose rule exists to stop, at 1.5 MB. What does
not rot went into docs/code-inspections.md instead: which inspections are off,
which findings were read and refused, and what the new tests cover.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@theAndreas
theAndreas merged commit 1d64b5d into master Sep 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@theAndreas @AndreasBurnickl