Skip to content

Support parenthesized sub-expressions - #5

Open
eugenefvdm wants to merge 2 commits into
omacom-io:masterfrom
eugenefvdm:add_brackets
Open

Support parenthesized sub-expressions#5
eugenefvdm wants to merge 2 commits into
omacom-io:masterfrom
eugenefvdm:add_brackets

Conversation

@eugenefvdm

@eugenefvdm eugenefvdm commented Aug 16, 2026

Copy link
Copy Markdown

Eg: (2 + 3) × 4 gives 20

eugenefvdm and others added 2 commits August 16, 2026 21:45
( and ) group terms and override operator precedence. "(" only opens
a fresh group -- at the start, after an operator, or after another
"(" -- and is otherwise ignored rather than guessing at an implied
operator; two groups need an explicit operator between them.
Evaluation moves from a flat multiply-then-add fold to a small
recursive-descent parser so nested groups compute correctly.

Co-authored-by: Cursor <cursoragent@cursor.com>
pressOperator already turns a leading operator at the top of the line into an implicit zero, so "- 3 + 4 =" gives 1. That branch only fired on an empty token list, so the same keys inside a group fell through to the plain append, put an operator first in the group, and parseFactor rejected it: "( - 3 + 4 ) =" gave Error, and so did the "( - 1 + 4 )" half of a longer expression. The ± key was the only way to enter a negative operand inside parentheses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed this against master. The parser itself is sound: I checked every paren-free path and the recursive-descent evaluator accepts exactly the grammar the old fold did, with the same precedence, the same rejection of malformed and even-length token lists, and the same ok == false on division by zero and non-finite totals. ./bin/build is warning-clean and ./bin/test is green. Nesting, unbalanced input and a stray ) all behave as your tests claim.

I pushed one fix, 1fa9f8a. pressOperator already seeds a leading operator at the top of the line with an implicit zero, so - 3 + 4 = gives 1, but that branch only fired on an empty token list. Inside a group the operator landed first in the group and parseFactor rejected it, so ( - 3 + 4 ) = gave Error, and so did the second half of ( 2 + 3 ) × ( - 1 + 4 ) =. ± was the only way to get a negative operand into parentheses. The branch now also fires after (, which makes ( × 3 ) come out as 0 the same way × 3 = already does. Test added; it fails without the change.

Four things I did not touch, because they are all one design question — currentValue() and pressPercent() do not know about groups — and the answer is the maintainer's:

  1. Percent inside an open group loses the running total. ( 200 + 10 % ) = gives 200.1. pressPercent builds leftSide as the tokens minus the pending +, which leaves ( 200; that does not parse, so baseOk is false and the percent falls back to 10 ÷ 100. The README documents 200 + 10 % = as 220, and it still is outside parentheses.
  2. currentValue() returns the whole expression's total only when the entire token list parses, and otherwise falls back to the last plain number in it. So ( ( 2 + 3 ) displays 3 rather than 5 after the inner group closes, and ( 2 + 3 ) × drops the display from 5 back to 3. That fallback also feeds percent: ( 2 + 3 ) + % = gives 5.15 where the equivalent 5 + % = gives 5.25.
  3. % right after a closed group puts the percentage in m_entry while the group stays in m_tokens, so = appends it as a bare operand next to ). ( 2 + 3 ) % = is Error, and ( 2 + 3 ) % × 2 = is too.
  4. Nothing bounds the nesting depth. Every ( is accepted after another (, and the parser recurses once per level on each keypress because display() re-evaluates. Compiling this backend at -O2 against an 8 MB stack, a run of unmatched ( segfaults somewhere between 20,000 and 30,000 of them — far past anything a person types, but there is no cap at all and the failure mode is a crash rather than an ignored key.

Two smaller notes. 2 ( 3 + 4 ) = gives 27: the ( is ignored per your stated rule, but the digits then fuse into 23, so the answer is silently wrong rather than refused — your own test pins 3 ( 4 + 5 ) = 39, so I assume that is intentional, but it is worth confirming. And in the README the period sits inside the code span, so it renders as 20. rather than 20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants