Stream rewritten SPDX identifiers - #28
Merged
Merged
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The streaming implementation matches existing String() precedence/parenthesization rules (including WITH wrapping under OR), preserves callback order, and is backed by differential fuzzing plus existing rewrite-order tests.
Pull request overview
This pull request optimizes RewriteIdentifiers by streaming rewritten SPDX expression output directly into a strings.Builder instead of cloning and re-rendering a second expression tree, while preserving the existing normalization/precedence rules and rewrite callback order.
Changes:
- Replace clone-based rewriting with a streaming
writeRewritten(*strings.Builder, rewrite, parentToken)implementation across expression node types. - Add a fuzz test to differential-check streaming output vs a clone-based reference implementation, plus a size-scaled benchmark.
- Update the
Expressioninterface to support streaming rewriting.
File summaries
| File | Description |
|---|---|
| rewrite.go | Implements streaming rewrite via writeRewritten methods and a small fast-path in RewriteIdentifiers. |
| rewrite_stream_test.go | Adds differential fuzzing against a clone-based implementation and benchmarks across expression sizes. |
| parse.go | Updates Expression interface to include writeRewritten, enabling streaming rewriting across node types. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
RewriteIdentifierscurrently builds a second expression tree and then renders it. This change writes rewritten identifiers directly to astrings.Builderwhile preserving callback order, precedence, license references, exceptions, special values, and input immutability.For an expression with 128 identifiers, the local benchmark falls from about 20.5 microseconds and 71.6 KB allocated to 4.7 microseconds and 3.0 KB. A differential fuzzer compares the streaming result with the former clone-based implementation across valid syntax trees.