Repository navigation
fix: compute nonut (unsigned) comparison, division and modulo as unsigned (#371) - #391
Conversation
…gned (#371) A nonut rizz above INT_MAX was compared, divided and taken modulo as a negative signed int: `3000000000 > 5` was false, `3000000000 / 2` gave 3647483648. Nothing told handle_binary_operation() an operand was unsigned -- get_expression_type() reports nonut rizz as plain VAR_INT, and the only check (OP_MOD's node->modifiers.is_unsigned) read the parse-time modifiers of the operation node, which are almost never set. Add expression_is_unsigned(), the signedness companion of expression_is_long(), and apply C's usual arithmetic conversions with it: - handle_binary_operation(): unsigned division, modulo and <, >, <=, >= when either int operand is unsigned (and neither is 64-bit). The INT_MIN / -1 overflow guard now sits after the unsigned branch, as OP_MOD's already did. - An unsigned operand widens to float/double through unsigned int. - evaluate_expression_long() zero-extends a 32-bit unsigned value, so mixing a nonut with a thicc/giga compares and adds correctly. Co-Authored-By: Claude Opus 5.5 <[email protected]>
leo-aa88
left a comment
leo-aa88
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
The arithmetic itself is right. It applies C's usual arithmetic conversions as written, including the unintuitive a > -1 → false. The INT_MIN / -1 guard is now ordered after the unsigned branch, which is exactly what the old comment demanded. Zero-extension in evaluate_expression_long() and the unsigned→double widening are correct. Deleting the dead node->modifiers.is_unsigned check is good: it only ever fired when an operation happened to be parsed inside a nonut declaration.
The problem is how signedness is found. expression_is_unsigned() is a fifth partial, per-evaluation, recursive type walker, alongside get_expression_type, get_expression_pointer_level, expression_is_long and the static mirrors. Each covers a different subset of node shapes. That causes one correctness hole and one hot-path regression.
1. *(p + i) on a nonut rizz * is still computed as signed: the issue's exact bug, through the standard pointer-read form
BLOCKING. CONFIRMED.
The description says signedness is "resolved from the declared descriptor of each operand form (identifier, array element, *p, …)". The OP_DEREFERENCE arm handles only a bare identifier operand:
ASTNode *operand = node->data.unary.operand;
if (!operand || operand->type != NODE_IDENTIFIER)
return false;So:
nonut rizz xs[2] = {1, 3000000000};
nonut rizz *p = &xs[0];
nonut rizz *q = &xs[1];
yapping("%d", *q > 5); 🚽 1 (fixed)
yapping("%d", *(p + 1) > 5); 🚽 0 (still wrong)
yapping("%u", *(p + 1) / 2); 🚽 3647483648 (the issue's exact wrong output)*(p + i) is how this language reads through a pointer: it's the form the #389 issue text cites as "what works today". **pp (pointer_level == 1 is required) and anything that isn't a bare name fall through the same way. The same object reads as unsigned or signed depending on how the address was spelled. ಠ_ಠ
The fix isn't another special case in this arm. A dereference yields the pointee of its operand's pointer type, so resolve the descriptor of the pointer expression (through +/- with an integer, as get_expression_pointer_level() already does for the level) and test the pointee. Add *(p + 1) and a **pp case to the fixture. The current fixture only covers *p, the one shape that's special-cased.
2. Every int operation now pays for a recursive signedness walk, about 1.5× slower on plain rizz arithmetic
MAJOR. Measured.
handle_binary_operation() computes is_unsigned for every VAR_INT operation, including +, -, *, == and !=, where your own comment says the bits are identical either way. That is two expression_is_long() calls plus up to two expression_is_unsigned() calls. Both recurse through the whole operand subtree with a get_variable() lookup per identifier, and they run again at every nested operation, so cost grows with depth² per evaluation. evaluate_expression_long() now also runs expression_is_long(node) && expression_is_unsigned(node) on entry, at every recursion level.
A loop of signed rizz arithmetic only (no nonut anywhere), 20k iterations, user time over 5 interleaved runs, same sanitized build flags:
main 0.55 0.60 0.63 0.65 1.01 (median 0.63 s)
#391 0.81 0.92 0.93 0.97 1.31 (median 0.93 s)
That is about +48% on the interpreter's hottest path, paid entirely by programs that never mention nonut. The signedness of an expression is a static property of declared descriptors: it can't change between iterations, yet it is recomputed on each one.
At minimum, compute is_unsigned lazily, only in the arms where it changes the result (/, %, <, >, <=, >=, and float widening). The better fix is to resolve the expression's descriptor once (semantic analysis or first evaluation) and cache it on the node. That would also remove the walker-coverage drift behind finding 1.
3. Minor
- The
NODE_FUNC_CALLarm can never fire today, becausecreate_function()zeroes return modifiers. The PR says so in its description, but nothing in the code does: a reader ofexpression_is_unsigned()will assumenonut rizz f()works. Put that caveat in a comment in the code. OP_MODchanged behavior in a second way:nonut rizz r = x % 3;with a signed negativexused to compute an unsigned modulo (because of the parse-time node modifier) and now computes a signed one, then converts it. The new behavior is the C-correct one (-7 % 3 == -1→4294967295), but it is an observable change no fixture locks in. Add one.
VERDICT
The arithmetic is right; the inference that decides when to use it is incomplete and expensive. The root cause is architectural: there is no single "static descriptor of this expression" function. Each property (type, pointer level, long, now unsigned) gets its own hand-rolled recursive walker over a slightly different subset of node shapes, evaluated again on every execution. Finding 1 is that drift producing a wrong answer, and finding 2 is the same design producing a ~1.5× slowdown on code that doesn't use the feature.
The tests only check that the implementation agrees with itself. Every operand form in the fixture is one the walker special-cases, so a walker that missed *(p + i) passes. Fix #1 with a descriptor-based dereference rule plus fixtures, and at least make is_unsigned lazy for #2. Not mergeable as is.
| case OP_POST_INC: | ||
| case OP_POST_DEC: | ||
| return expression_is_unsigned(node->data.unary.operand); | ||
| case OP_DEREFERENCE: |
There was a problem hiding this comment.
BLOCKING. Only *name is recognized. *(p + 1) > 5 on a nonut rizz *p pointing at 3000000000 still gives 0, and *(p + 1) / 2 gives 3647483648, the issue's exact wrong output. **pp also falls through. Resolve the pointee from the operand's pointer descriptor (through +/- with an integer) instead of special-casing the identifier shape. (Finding 1.)
| either way; division, modulo and the ordering comparisons do not, so | ||
| those arms below branch on this. A 64-bit operand would make the | ||
| operation a signed long instead, which is not computed here. */ | ||
| bool is_unsigned = promoted_type == VAR_INT && |
There was a problem hiding this comment.
MAJOR. This runs for every int op, including +/-/*/==/!=, where the comment above says the result is bit-identical. Two expression_is_long() plus up to two expression_is_unsigned() full-subtree recursions on every evaluation, at every nesting level. Measured about +48% user time on a loop of plain signed rizz arithmetic. Compute this only in the /, %, ordering and float-widening arms, or better, resolve it once and cache it on the node. (Finding 2.)
| by zero-extension, as C converts unsigned int to long (#371). Its | ||
| own arithmetic stays 32-bit, so `u - 1` on 0 is 4294967295 here, | ||
| not -1. */ | ||
| if (!expression_is_long(node) && expression_is_unsigned(node)) |
There was a problem hiding this comment.
MAJOR. Two more recursive walks on entry to every evaluate_expression_long() call, and this function recurses, so a nested 64-bit expression pays O(depth²) walks per evaluation. Same fix as in handle_binary_operation(). (Finding 2.)
…ess (#371) Review of #391: - `*(p + i)`, `**pp`, `*ptrs[i]` on a `nonut rizz *` were still computed as signed: the dereference arm only recognized a bare `*name`. Replace the per-shape cases with static_value_descriptor(), which reads the declared descriptor of any value-denoting expression -- `*E` is E's descriptor one pointer level down, `p ± i` keeps p's -- and test the pointee from that. - Every int operation re-walked both operand subtrees for signedness on every evaluation (~2x slower on plain signed arithmetic). Signedness follows from declarations, so memoize it on the node (ASTNode.signedness_memo), as check_and_mark_identifier() does for its symbol check. Back to parity with main on a signed-only loop. - Note in the code that a call never reads as unsigned yet, since create_function() records no return modifiers. - Fixture: the pointer-expression shapes, and a signed `%` stored into a nonut (-7 % 3 -> 4294967295; the old node-modifier check computed an unsigned modulo of the signed operand and gave 0). Co-Authored-By: Claude Opus 5.5 <[email protected]>
leo-aa88
left a comment
leo-aa88
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
Re-review of cc6d106. Both previous findings are fixed properly, at their root causes:
*(p + i)signedness — fixed.static_value_descriptor()treats a dereference as "the operand's descriptor one level down", so*(p + 1),*(0 + q),**pp,*ptrs[1]and*(&x)all resolve. My earlier repro now gives1/1500000000, and the fixture covers the shapes the old special case missed.- Hot-path cost — fixed. Signedness is memoized on the node. On the same signed-only
rizzloop, the gap frommainis now inside run-to-run noise: the fastest PR run (0.94 s) beat the fastestmainrun (1.03 s), where before it was +48%.
The signed-%-into-nonut fixture I asked for is there too.
The memo makes one thing much more important than before: operation_is_unsigned() is now the single authority on whether an operation is done in unsigned int, and it answers wrong in two cases.
1. operation_is_unsigned() trusts literal modifiers it itself calls parser state, so the same expression wraps or doesn't depending on where it's written
BLOCKING. CONFIRMED.
nonut rizz a = 3000000000;
thicc rizz t2 = a * 2; 🚽 6000000000 ← no 32-bit wrap
thicc rizz t3 = 0;
t3 = a * 2; 🚽 1705032704 ← wraps, as C requiresThe expression and target type are identical, but the results differ. In C, a * 2 is unsigned int arithmetic, which wraps (well-defined) to 1705032704 before converting to long. The PR description promises exactly this: "the 32-bit arithmetic itself still wraps at 32 bits ... as in C".
Root cause, from gdb in operation_is_unsigned() for the declaration's a * 2:
node->data.op.right->type = NODE_INT (the literal 2)
node->data.op.right->modifiers = { ..., is_long_long = true, ... }
The parser stamped the enclosing thicc declaration's modifiers onto the literal. operation_is_unsigned() asks !expression_is_long(right), expression_is_long()'s NODE_INT arm reads exactly those modifiers, and so the operation is classified as "signed 64-bit" and skips the wrap. The new expression_is_unsigned() comment states the problem itself: "the modifiers create_int_node() stamps on it are the parser's state at the time, not the literal's type". The new function then makes its decision through a helper that reads those same modifiers. ಠ_ಠ
Under the memo, the wrong answer is also permanent for that node.
The fix belongs in the literal's type, not in another special case. A decimal literal is int if its value fits and long/long long otherwise (C11 6.4.4.1). Derive that from the value, in expression_is_long()'s NODE_INT arm or at create_int_node(), so no evaluator ever sees parser state as a type. Add the declaration-initializer form to the fixture: every 64-bit line in it today is inside yapping(...), where the literal is unstamped, so a thicc rizz x = <nonut arithmetic>; initializer is the case it misses.
2. A double-valued operation is classified as unsigned int, which silences an existing diagnostic and returns garbage
MAJOR. CONFIRMED.
nonut rizz a = 3000000000;
thicc rizz t = a + 0.5;
yapping("%lld", t);main:Error: Cannot use double in integer context, then-1294967296.- this head: no diagnostic,
2147483648.
operation_is_unsigned() checks pointer levels and 64-bit-ness but never that both operands are integers, so a + 0.5 (computed in double) is classified as "carried out in unsigned int". evaluate_expression_long()'s new early return then routes the double expression through evaluate_expression_int(), an out-of-range double→int conversion, and zero-extends the result. The function's own contract ("whether the binary operation ... is carried out in unsigned int") is false for any float/double operand, and the bad answer is memoized.
Require both operands to be integer-typed (get_expression_type() in the int family) before classifying the operation as unsigned. The correct result, 3000000000, then comes from the long path's existing double handling, or at worst you get back the old diagnostic, instead of a silent wrong value.
VERDICT
The architecture is now right: one declaration-driven descriptor walk, with dereference as a single rule, memoized because the answer is static. Both remaining defects come from the same place. operation_is_unsigned() decides "is this unsigned int arithmetic?" without establishing that both operands are integer-typed ints. It takes expression_is_long()'s parser-state view of literals as truth, and it never checks for floating operands. Because the answer is now memoized and is the only authority, a wrong classification silently yields a wrong value on every evaluation (implementation, resting on a pre-existing literal-typing flaw this PR newly depends on).
The tests check the implementation against itself. No fixture puts nonut arithmetic in a 64-bit declaration initializer or mixes a nonut operand with a floating literal, and those are exactly the two places it breaks. Fix the literal typing and the operand-type guard, add both cases to nonut_binary_ops, and this is close to mergeable. Not mergeable as is.
| (is_int_arithmetic_op(op) || is_int_comparison_op(op)) && | ||
| get_expression_pointer_level(node->data.op.left) == 0 && | ||
| get_expression_pointer_level(node->data.op.right) == 0 && | ||
| !expression_is_long(node->data.op.left) && |
There was a problem hiding this comment.
BLOCKING. expression_is_long()'s NODE_INT arm reads the literal's parser-stamped modifiers, which expression_is_unsigned()'s own comment says are parser state, not the literal's type. Inside thicc rizz t2 = a * 2; the literal 2 carries is_long_long = true (checked in gdb), so this classifies the op as signed-64, skips the 32-bit wrap, and memoizes that: t2 = 6000000000, while the identical t3 = a * 2; gives the C-correct 1705032704. Type literals by value (C11 6.4.4.1). (Finding 1.)
| Memoized on the node like check_and_mark_identifier()'s symbol check: | ||
| signedness follows from declarations, so it cannot change between | ||
| evaluations, and every int operation asks. */ | ||
| static bool operation_is_unsigned(ASTNode *node) |
There was a problem hiding this comment.
MAJOR. Nothing here requires both operands to be integers, so a + 0.5 (a double operation) is memoized as "carried out in unsigned int". thicc rizz t = a + 0.5; then silently yields 2147483648, where main at least reported Cannot use double in integer context. Gate on both operands being int-family typed. (Finding 2.)
| by zero-extension, as C converts unsigned int to long (#371). Its | ||
| own arithmetic stays 32-bit, so `u - 1` on 0 is 4294967295 here, | ||
| not -1. */ | ||
| if (expression_is_unsigned(node)) |
There was a problem hiding this comment.
MAJOR. With finding 2's misclassification, this early return sends a double-valued expression through evaluate_expression_int(), an out-of-range double→int conversion, and zero-extends the result. Its correctness depends entirely on operation_is_unsigned() never saying yes to a floating operation, which it currently does. (Finding 2.)
…d int (#371) Second review of #391: - create_int_node()/create_long_node() stamped the enclosing declaration's parser modifiers onto the literal, so in `thicc rizz t = a * 2;` the `2` read as 64-bit, expression_is_long() classified `a * 2` as a signed long and skipped the unsigned 32-bit wrap (6000000000), while the same `t = a * 2;` wrapped (1705032704). A literal's width and signedness now come from its value (C11 6.4.4.1): int if it fits, long long otherwise, never unsigned. - operation_is_unsigned() never required integer operands, so `a + 0.5` was memoized as unsigned int arithmetic and evaluate_expression_long() sent the double through evaluate_expression_int(). Require both operands to be int-family typed. - evaluate_expression_long() now converts a floating operation's double value (as C does) instead of re-evaluating its operands as integers, which reported "Cannot use double in integer context" for any `thicc rizz t = x + 0.5;` and dropped the double operand. - Fixture: nonut arithmetic in a thicc initializer vs an assignment, and a nonut + double operation converted to thicc. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 participant
Description
A
nonut rizzaboveINT_MAXwas compared, divided and taken modulo as a negative signed int:a = 3000000000)a > 50("small")1a / 236474836481500000000a % 704a > t(thicc rizz t = 5)01a + t-12949672913000000005a + 0.5-1294967295.53000000000.5Root cause.
get_expression_type()reportsnonut rizzas plainVAR_INT, and its value sits in the sameintslot, so nothing toldhandle_binary_operation()that an operand's bits were unsigned. The one existing check,OP_MOD'snode->modifiers.is_unsigned, read the parse-time modifiers of the operation node. Those are only set when the operation happens to be parsed inside anonutdeclaration, so%was wrong too in any other position. The code comment inOP_DIVIDEalready noted this.Fix. Add
expression_is_unsigned()inast.c. It is the signedness counterpart of the existingexpression_is_long(), resolved from the declared descriptor of each operand form (identifier, array element,*p, struct field, user-function return). On top of it, apply C's usual arithmetic conversions:handle_binary_operation(): unsigned/,%,<,>,<=and>=when either int operand is unsigned and neither is 64-bit.+,-,*,==and!=already produce the same bits either way (they wrap throughunsignedsince [Bug]: int arithmetic (+, -, *, unary -) is signed-overflow UB — interpreter relies on undefined behavior for wrapping #280). Their results now count as unsigned, though, so(a + 1) > 5and(a * 1) / 2are right too. TheINT_MIN / -1overflow guard now runs after the unsigned branch, asOP_MOD's already did.0x80000000 / 0xFFFFFFFFis a well-defined unsigned division and now returns0instead of a "most negative rizz" diagnostic. The old deadnode->modifiers.is_unsignedcheck is gone.unsigned int.evaluate_expression_long(): a 32-bit unsigned value is zero-extended, as C convertsunsigned inttolong. That makesnonutmixed withthicc/gigacompare and add correctly. The 32-bit arithmetic itself still wraps at 32 bits, so(z - 1) + twithz = 0is4294967300, as in C.These follow C exactly, including the less obvious case:
a > -1is now false, because-1converts to4294967295.Not changed, and out of scope:
create_function()never records return-type modifiers; it zeroes them, as its comment notes forgigareturns too. TheNODE_FUNC_CALLbranch ofexpression_is_unsigned()mirrorsexpression_is_long()'s, and will work once those modifiers are carried.nonut giga) arithmetic in the long path.nonuttochad/gigachadin an initializer or assignment. That is a conversion path, not a binary operation (cf. [Bug]: giga/thicc truncate to 32 bits when converted to chad/gigachad (float/double) #372).Related Issue
Fixes #371
Type of Change
Checklist
test_cases/*.brainrotfor language-visible behavior; a host-side C test wired intomake testfor internal APIs Brainrot source cannot reach). Required for every bug fix, feature, refactor, performance change, and breaking change — not optional, not "if applicable". See CONTRIBUTING.md ("TESTS ARE MANDATORY").make format-checklocally (ormake formatto fix)test_cases/nonut_binary_ops.brainrotcovers:%and all six comparisons;a > -1;+/*//results and unary-;0x80000000 / 0xFFFFFFFFcase;*p, and alit nonut rizzstruct field and array field;thiccand into float;goonloop condition;It uses
thicc, notgiga, for the 64-bit lines, becausegigais 32-bit on wasm32 (#177).make test: 543 passed.make format-check: clean.make valgrind: exit 0,ERROR SUMMARY: 0and no definite leaks on every fixture.🤖 Generated with Claude Code