Zero divider cpp review items - #205
Open
cbrisebois-lasp wants to merge 6 commits into
Open
Conversation
cbrisebois-lasp
force-pushed
the
zero-divider-cpp-review-items
branch
from
August 17, 2026 21:21
4c211b5 to
a4c202c
Compare
cbrisebois-lasp
force-pushed
the
zero-divider-cpp-review-items
branch
from
September 1, 2026 14:36
3c480d0 to
2ae817f
Compare
A formal review found the component stating as fact what its two divide-by-zero commands would do, when the outcome actually depends on the target, the compiler, and the flags a project builds with. The descriptions now say only that each command performs the division and reports what comes back, with IEEE 754 cited as a typical result rather than a guarantee, and the two new requirements are worded to match. Behavior claims are confined to the unit tests, the one place a configuration is fixed and known, so each of those tests names the Linux_Test target and the GNAT flags its assertion depends on. Those mechanisms were confirmed by running the tests rather than reasoned about: the integer division traps and GNAT's signal manager raises Constraint_Error, while the floating point division returns an infinity that -gnatVa rejects as invalid data. The old comments attributed the latter to assigning the result to a constrained float subtype excluding infinities; no such subtype exists, the result is a plain Short_Float, and that claim is gone. Templates were regenerated so the generated comments track the models, but the tester's new Packet.Representation include was left out as unrelated framework drift. No executable code changed.
Sending 0 or 1 as a magic number previously reached a command handler and came back as an invalid magic number, when the accurate cause is that the value is not a legal magic number at all. Modelling the magic number as a packed record whose field carries the 2 .. Unsigned_32'Last range moves that rejection into command validation, so such a command is now refused as an invalid command with the errant field identified. Execution is unaffected either way, only the reported cause changes. Packed_Magic_Number follows command_protector's Packed_Arm_Timeout, the framework's closest analogue, which likewise keeps its constrained type in the record preamble and uses the packed record as both a command argument and an event parameter. The range now has a single definition there, and the component preamble aliases it so the Init signature is untouched. The wire format does not move: a lone U32 sits where Packed_U32 sat, so generated ground definitions change only in the type name they reference. The new test builds each command with a valid magic number and then overwrites the first four argument bytes, because Pack cannot be asked to produce a value its field type excludes; the magic number being the first field of all three command arguments makes that uniform. The argument records moved into types/ alongside the new one, matching where the rest of the framework keeps component-local types.
The three command handlers opened identically: the same magic number check with the same rejection event and return, an announcement event, and the sleep that gives that announcement time to be written. Stage_Command now carries the whole sequence; the announcement is the only part that differs per command, so each handler passes its own, already constructed, and the helper sends it only once the magic number has passed. Sleep_Duration takes care of the one remaining triplicated expression, the conversion of the configured sleep into the announcement's event parameter. Both helpers live in the body rather than the spec and are called as Stage_Command (Self, ...) rather than through the prefixed notation, because only subprograms declared alongside the type are primitive operations; memory_stuffer's Do_Unarm is the existing example of the same shape. Checking the interlock first and returning early lets the if/else nesting go, which dedents the rest of each handler. The one behavioral nuance is that a handler now builds its announcement, timestamp included, before the magic number is checked; the event is discarded on a mismatch, so the emitted events, the sleep, and the returned statuses are all unchanged. Packed_U32 needs an explicit with now that Sleep_Duration names it in a return type, having previously been visible only through the generated base class. Return a command execution status from Stage_Command A Boolean answer made each handler translate the staging outcome back into the enumeration the command system actually speaks; returning Command_Execution_Status.E directly lets the Failure arm pass the staging result through unchanged. Each handler encloses its work in the Success arm instead of falling through past a null arm, so the case statement is the complete story of the handler: every staging outcome is handled explicitly and adjacently, and every path out of the handler is a return from one of its arms. Casing on the result rather than testing a Boolean also puts the compiler on the hook — any literal the enumeration grows must be handled at every call site, where an if not would keep compiling silently.
…tion Each of the three tests that expect an exception asserted False after sending the command, directly above an `exception when E : others` handler that caught the resulting Assert_Failure. A command that returned instead of raising therefore failed on the wrong assertion, reporting "Expected Constraint_Error but got SYSTEM.ASSERTIONS.ASSERT_FAILURE" rather than naming the actual problem. The send and the handler now sit in an inner block, the handler records that it ran, and the fall-through assertion moved below that block where nothing can intercept it. Confirmed by temporarily sending Raise_Exception_In_Cpp with a magic number that makes it return normally: the run now reports "Command returned without raising an exception." and fails only that test. No test changes its verdict, since both arrangements fail when a command returns unexpectedly; only the diagnostic differs. The assertion messages are kept rather than moved into comments as the style guide asks, because these strings are compiled into a unit test binary that is never linked into a flight image, so the reason for the rule does not apply here.
The coverage report shows the No_Exception event sends and the returns after them as unexecuted even though every test drives its handler into the C++ call. Each site now carries a comment stating why the tests cannot cover it, so a coverage review does not read the misses as untested behavior.
cbrisebois-lasp
force-pushed
the
zero-divider-cpp-review-items
branch
from
September 1, 2026 17:49
2ae817f to
5fbf3e1
Compare
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
1 participant
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.
No description provided.