parseMinor's '+ 0.5' rounding: the clang-tidy check that was pointing at it no longer does, and the question is still open

[Yaraslaut]

What happened

#663's fix (PR #676) bounds bankgui::fmt::parseMinor's input so an out-of-range amount is rejected instead of being converted out of range. To do that it splits the expression:

-    return static_cast<std::int64_t>(major * scale + 0.5);
+    const double minor = (major * scale) + 0.5;
+    if (!(minor < kMinorUnitsBound)) { return std::nullopt; }
+    return static_cast<std::int64_t>(minor);

The rounding behaviour is unchanged. What did change is that the line no longer matches bugprone-incorrect-roundings, which was reporting on it before:

$ clang-tidy -p build/clang-debug --extra-arg=-std=c++23 \
      --extra-arg=-Wno-missing-include-dirs --quiet \
      examples/bank/gui/controllers/Format.hpp      # pre-fix content, on 563502ab
/.../examples/bank/gui/controllers/Format.hpp:76:38: error: casting (double + 0.5) to integer leads to incorrect rounding; consider using lround (#include <cmath>) instead [bugprone-incorrect-roundings,-warnings-as-errors]

So a check that was pointing at this arithmetic has stopped pointing at it, without the question it was raising being answered. #663's filer had already flagged + 0.5 and labelled the concern weak; the triage said to decide it separately rather than fold an unmeasured rounding change into a UB fix, which is what PR #676 did. This is that separate ticket.

The substance of the check's complaint

static_cast<int64_t>(x + 0.5) is not round-half-away-from-zero. x + 0.5 is itself a rounded double operation, so a value just below a half can be rounded up to exactly the next integer by the addition and then truncate to the wrong side. The textbook witness is 0.49999999999999994, the largest double below 0.5: 0.49999999999999994 + 0.5 == 1.0 in IEEE-754 double, so the expression yields 1 where correct rounding yields 0. std::llround does not have that behaviour.

Whether any such value is reachable here is the open question, and it is a narrower one than the general case, because x is always major * scale for scale a power of ten and major a value QString::toDouble produced from user text. I have not searched for a reachable witness.

Verification status

• Reproduced: the clang-tidy finding on the pre-fix line, output above, on 563502ab with clang 22.1.8 and the clang-tidy job's own configure. Also reproduced that it no longer fires after PR bank/gui: bound parseMinor's input so an out-of-range amount is rejected rather than undefined (fixes #663) #676's split.
• Reproduced: that the rounding behaviour is unchanged by PR bank/gui: bound parseMinor's input so an out-of-range amount is rejected rather than undefined (fixes #663) #676 — parseMinor("0.005") == 1 before and after, and a sweep of 24576 doubles across the accept/reject boundary for decimals 0/1/2 gives identical accepted values.
• Not verified — this is the weak part: whether any input a user can type into the bank GUI actually reaches a double where x + 0.5 rounds to the wrong side. I did not search for one. It may well be unreachable, in which case the honest resolution is a comment saying so rather than a code change.
• Not verified: whether std::llround would be a drop-in. It rounds half away from zero, which for non-negative values matches the intent, but it is also UB on an out-of-range argument — so it would have to sit inside the bound PR bank/gui: bound parseMinor's input so an out-of-range amount is rejected rather than undefined (fixes #663) #676 added, not replace it.

What would change the verdict

• Close as invalid if someone shows the input domain cannot produce a witness — e.g. because QString::toDouble output times a power of ten can never land on a double of that shape. That argument needs writing down either way, since it is the thing the current code silently depends on.
• Close as valid and fix by replacing the expression with std::llround(major * scale) inside the existing bound, plus a test carrying the witness value.
• Re-open if bugprone-incorrect-roundings is later re-triggered by an unrelated edit to this function, which would mean the split was load-bearing for the suppression rather than incidental.

Found while working #663 (PR #676). Filed, not folded.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Triage: valid — the weak half is now measured, and the rounding is genuinely wrong

You labelled the + 0.5 question weak because you had not searched for a reachable witness. There is one, and it is typeable.

  x                = 0.49999999999999994449
  x < 0.5          = true
  x + 0.5          = 1
  (int64)(x + 0.5) = 1      <- correct rounding gives 0
  std::llround(x)  = 0

0.49999999999999994 is the double immediately below 0.5. Adding 0.5 rounds up to exactly 1.0 in IEEE-754, so the truncating cast yields 1 where the value should round to 0. That is precisely what bugprone-incorrect-roundings exists to catch, and it is why the check was pointing at that line.

Through parseMinor, from a string a user can type:

  "0.004999999999999999"  -> major=0.0049999999999999992  scaled=0.49999999999999994  minor=1   <-- WRONG
  "0.0049999999999999994" -> same                                                     minor=1   <-- WRONG
  "0.005"                 -> scaled=0.5                                               minor=1   (correct)

So an amount that scales to just under half a minor unit is charged a whole one. Not a constructed double — a 20-character decimal string through QString::toDouble.

triage: valid, bug, area: ladder.

Scope, honestly

The impact is one minor unit on a pathological input, and nobody is typing 19 significant figures into a payment field by accident. This is not #663 — that was undefined behaviour on ordinary input; this is a wrong answer on an extreme one. Worth fixing because the fix is smaller than the argument, not because it is dangerous.

The more durable point is the one your issue actually raises: the check that was pointing at this line stopped matching after #663's split, so the tree now contains a real rounding defect that clang-tidy no longer reports. A finding that disappears because the expression moved — while the behaviour is unchanged — is worse than one that was never reported, because the green tick now asserts something false.

For whoever takes it

• std::llround(scaled) is the fix and is demonstrably correct on the witness above (0 where + 0.5 gives 1). It also removes the construct the check was complaining about, rather than hiding it.
• Mind the interaction with bank GUI: parseMinor casts an unbounded double to int64_t; 1e30 in any amount field is UB #663's bound. parseMinor now rejects anything not below 0x1p63 before converting; llround on a value inside that range is well-defined, but the order of guard-then-round must be preserved or the UB bank GUI: parseMinor casts an unbounded double to int64_t; 1e30 in any amount field is UB #663 closed comes back.
• The test must use the witness. A rounding test with 0.005 and 0.004 passes both before and after and proves nothing — the input has to be one where the two disagree.

What would change the verdict

Close as wontfix if the considered answer is that sub-ulp inputs to a currency field are out of contract — which is defensible. But then say so in the code, because the next person to read + 0.5 will not know it was a decision.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW