Skip to content

refactor(symbols): Result lives only on AnnotationStore - #78

Merged
rieske merged 2 commits into
masterfrom
feature/result-on-store
Jul 26, 2026
Merged

refactor(symbols): Result lives only on AnnotationStore#78
rieske merged 2 commits into
masterfrom
feature/result-on-store

Conversation

@rieske

@rieske rieske commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • ValueEntry lives in symbols/ (re-export from semantic_analyzer/ValueEntry.h)
  • AnnotationStore owns ValueSlot::Result / Lvalue (no dual-write node cache)
  • Expression Result API is store-only: setters/getters/valueType take AnnotationStore&
  • Operand / initializer / lvalue helpers pass the store through SA and CG
  • Drop unused PreOperation slot

Stack

Phase A PR 3/3 — base: #77 AddressBaseMode → #76 CallPlan

Test plan

  • Full suite (astTest, symbolsTest, functional shards)
  • CI + Coveralls

@mergify

mergify Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@rieske
rieske force-pushed the feature/result-on-store branch 2 times, most recently from d209754 to f39ba54 Compare July 26, 2026 06:30
@rieske
rieske force-pushed the feature/address-base-mode branch from 2293396 to ce35301 Compare July 26, 2026 07:02
@rieske
rieske force-pushed the feature/result-on-store branch from f39ba54 to 06b873c Compare July 26, 2026 07:09
@rieske rieske changed the title refactor(symbols): Result dual-write onto AnnotationStore refactor(symbols): Result lives only on AnnotationStore Jul 26, 2026
@coveralls

coveralls commented Jul 26, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 93.287% (+0.04%) from 93.246% — feature/result-on-store into master

@rieske

rieske commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Code review

No blocking issues found. Store-only Result migration is complete and consistent.

Non-blocking notes (below confidence threshold for required fixes):

  1. AnnotationStore class comment still says Result/Lvalue both live on the store; production Lvalues remain node fields until a follow-up (ValueSlot comment already admits this).
  2. getResultSymbol is soft-null while CG largely assumes presence after successful SA; AnnotationStore::result() still asserts — document or unify if you want a stricter contract.
  3. clearEmptiesAll unit test does not exercise Result/Lvalue clear (covered by ExpressionDualTypeTest).

🤖 Generated with Grok code-review

@rieske
rieske force-pushed the feature/address-base-mode branch from ce35301 to 52bc4e1 Compare July 26, 2026 08:47
@rieske
rieske force-pushed the feature/result-on-store branch from a7a4507 to d8bd5fe Compare July 26, 2026 08:47
Base automatically changed from feature/address-base-mode to master July 26, 2026 08:51
rieske added 2 commits July 26, 2026 11:52
Move ValueEntry into symbols/ and store ValueSlot::Result (and Lvalue)
on AnnotationStore. Expression no longer dual-writes a node cache:
has/get Result, valueType, and helper accessors all take the store.
Drop unused PreOperation slot; SA/CG and tests use the store-only API.
Document Result-now / Lvalue-TBD on the store; getResultSymbol asserts
via store.result() to match post-SA CG usage; extend clearEmptiesAll to
cover Result and Lvalue slots.
@rieske
rieske force-pushed the feature/result-on-store branch from d8bd5fe to cdffa16 Compare July 26, 2026 08:52
@rieske
rieske merged commit a62bbb8 into master Jul 26, 2026
4 checks passed
@rieske
rieske deleted the feature/result-on-store branch July 26, 2026 08:57
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