Evaluate range pattern conditions lazily - #540
Conversation
Range patterns (begpat, endpat) used to evaluate both conditions on every record, which is wrong when the conditions have side effects (e.g. "a++ == 2, a++ == 5"). Per POSIX, the start condition must be evaluated only while outside the range, and the end condition only once the range has started (including on the record that starts it, so a range can begin and end on the same record). ConditionPairAst now compiles to conditional jumps around the two condition expressions, using the new CONDITION_PAIR_IN_RANGE, CONDITION_PAIR_ENTER and CONDITION_PAIR_LEAVE opcodes, appended at the end of the Opcode enum to preserve serialized numeric identifiers. The legacy CONDITION_PAIR opcode remains supported for precompiled tuple streams. Fixes #115 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1d1f74a83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The start condition is evaluated only while outside the range, and the | ||
| // end condition only once the range has started (including on the very | ||
| // record that starts it, so a range can begin and end on the same record) |
There was a problem hiding this comment.
Because this changes observable AWK output whenever range conditions have side effects, it is an end-user-facing behavior change, but the commit updates neither README.md nor any src/site documentation. Document the corrected range-pattern semantics in both required locations.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Documented in the compatibility page's Detailed Behavior Notes (new "Range patterns" section). README already delegates to that page for detailed behavior notes — it links to https://jawk.io/compatibility.html for exactly this — and its POSIX support claim now simply holds, so no README change is needed.
| @Test | ||
| public void testEnterLeave() { |
There was a problem hiding this comment.
This newly added test constructs ConditionPair and uses direct JUnit assertions, bypassing the project-required AwkTestSupport setup and assertion flow. Rewrite the coverage using the helper, or rely on the new helper-based integration cases instead.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed the direct-JUnit test; the new AwkTestSupport-based cases in AwkTest#testRanges exercise isWithin()/enter()/leave() through the interpreter.
Add the corrected range-pattern semantics to the compatibility page's detailed behavior notes (README already links there for behavior notes), and remove the ConditionPair accessor unit test in favor of the AwkTestSupport-based integration coverage in AwkTest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Fixes #115
Problem
Range patterns (
begpat, endpat) evaluated both conditions on every record, which produces wrong results when the conditions have side effects:on input
1..10printed2 3instead of3 4 5.Fix
ConditionPairAstnow compiles to conditional jumps so that:/b/, /b/behavior is unchanged).This is implemented with three new opcodes —
CONDITION_PAIR_IN_RANGE,CONDITION_PAIR_ENTER,CONDITION_PAIR_LEAVE— carrying a per-pattern identifier, appended at the end of theOpcodeenum to preserve serialized numeric identifiers. The legacyCONDITION_PAIRopcode is kept in the AVM for precompiled tuple streams, andAwkTuples.conditionPair()is deprecated.Tests
a++ == 2, a++ == 5→3 4 5), an end-condition side-effect case, and two independent ranges in one script, all verified against gawk 5.0.ConditionPair.isWithin()/enter()/leave()accessors.mvn verifypasses: surefire green, checkstyle/PMD/SpotBugs clean.🤖 Generated with Claude Code