Add Iterator.concat support#253
Merged
Merged
Conversation
- Implement lazy `Iterator.concat(...items)` for iterable sequencing - Add JS coverage, benchmark cases, and built-in docs - Register the new iterator concat value in the runtime
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements Iterator.concat: adds a new Pascal unit and iterator class for lazy concatenation, registers a static Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Host as Iterator.concat
participant ConcatIter as TGocciaConcatIteratorValue
participant InnerIter as Inner Iterator
participant Source as Source Iterable
Client->>Host: call concat(item1, item2, ...)
Host->>Host: validate args, resolve Symbol.iterator / strings
Host->>ConcatIter: Create(iterables)
Host-->>Client: return ConcatIter (iterator)
Client->>ConcatIter: next()
ConcatIter->>ConcatIter: if no active inner -> OpenNextIterator()
ConcatIter->>Source: lazily obtain inner iterator
ConcatIter->>InnerIter: next()
InnerIter-->>ConcatIter: {value, done}
alt done = false
ConcatIter-->>Client: {value, done: false}
else done = true
ConcatIter->>ConcatIter: Close active inner, advance to next source or finish
opt next source exists
ConcatIter->>Source: open next inner iterator (lazily)
ConcatIter->>InnerIter: next()
InnerIter-->>ConcatIter: {value, done}
ConcatIter-->>Client: {value/finished}
end
alt no sources left
ConcatIter-->>Client: {value: undefined, done: true}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Values.Iterator.Concat.pas (1)
189-194: Consider nilling FCurrentIterator after Close for idempotency.If
Closeis called twice,FCurrentIterator.Closewill be invoked twice. While most iterator Close implementations are tolerant of this, settingFCurrentIterator := nilafter closing would make the behavior more robust.♻️ Optional fix for Close idempotency
procedure TGocciaConcatIteratorValue.Close; begin inherited; if Assigned(FCurrentIterator) then + begin FCurrentIterator.Close; + FCurrentIterator := nil; + end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Iterator.Concat.pas` around lines 189 - 194, The Close method of TGocciaConcatIteratorValue should be made idempotent by nulling FCurrentIterator after closing it; update TGocciaConcatIteratorValue.Close to check Assigned(FCurrentIterator), call FCurrentIterator.Close and then set FCurrentIterator := nil so subsequent Close calls won't attempt to close the same iterator again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Values.Iterator.Concat.pas`:
- Around line 189-194: The Close method of TGocciaConcatIteratorValue should be
made idempotent by nulling FCurrentIterator after closing it; update
TGocciaConcatIteratorValue.Close to check Assigned(FCurrentIterator), call
FCurrentIterator.Close and then set FCurrentIterator := nil so subsequent Close
calls won't attempt to close the same iterator again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30d8ee6c-c811-44b6-9d8c-878656d35194
📒 Files selected for processing (5)
benchmarks/iterators.jsdocs/built-ins.mdtests/built-ins/Iterator/concat.jsunits/Goccia.Values.Iterator.Concat.pasunits/Goccia.Values.IteratorValue.pas
frostney
added
the
spec compliance
- Nil out the current iterator after closing it - Avoids retaining a stale iterator reference in concat iteration
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results281 benchmarks Interpreted: 🟢 237 improved · 🔴 9 regressed · 🆕 7 new · 28 unchanged · avg +7.3% arraybuffer.js — Interp: 🟢 13, 🔴 1 · avg +8.9% · Bytecode: 🟢 6, 8 unch. · avg +2.1%
arrays.js — Interp: 🟢 19 · avg +7.0% · Bytecode: 🟢 13, 6 unch. · avg +4.8%
async-await.js — Interp: 🟢 6 · avg +8.3% · Bytecode: 🟢 4, 2 unch. · avg +2.4%
classes.js — Interp: 🟢 24, 7 unch. · avg +5.2% · Bytecode: 🟢 8, 🔴 1, 22 unch. · avg +3.5%
closures.js — Interp: 🟢 11 · avg +8.5% · Bytecode: 🟢 5, 6 unch. · avg +3.2%
collections.js — Interp: 🟢 10, 2 unch. · avg +14.4% · Bytecode: 🟢 6, 🔴 1, 5 unch. · avg +3.0%
destructuring.js — Interp: 🟢 21, 1 unch. · avg +7.3% · Bytecode: 🟢 16, 6 unch. · avg +6.5%
fibonacci.js — Interp: 🟢 8 · avg +14.5% · Bytecode: 🟢 3, 🔴 3, 2 unch. · avg -0.7%
for-of.js — Interp: 🟢 7 · avg +6.4% · Bytecode: 🟢 7 · avg +9.3%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 20, 🆕 7 · avg +8.1% · Bytecode: 🟢 16, 🔴 2, 🆕 7, 2 unch. · avg +4.3%
json.js — Interp: 🟢 20 · avg +8.8% · Bytecode: 🟢 14, 🔴 1, 5 unch. · avg +3.1%
jsx.jsx — Interp: 🟢 16, 5 unch. · avg +4.6% · Bytecode: 🟢 12, 🔴 2, 7 unch. · avg +2.7%
modules.js — Interp: 🟢 9 · avg +10.7% · Bytecode: 🟢 2, 🔴 7 · avg -7.6%
numbers.js — Interp: 🟢 10, 1 unch. · avg +11.9% · Bytecode: 🟢 4, 🔴 1, 6 unch. · avg -1.3%
objects.js — Interp: 🟢 3, 🔴 3, 1 unch. · avg +0.2% · Bytecode: 🟢 5, 2 unch. · avg +2.5%
promises.js — Interp: 🟢 10, 2 unch. · avg +6.2% · Bytecode: 🟢 7, 5 unch. · avg +3.8%
regexp.js — Interp: 🟢 10, 1 unch. · avg +7.7% · Bytecode: 🟢 5, 🔴 1, 5 unch. · avg +2.2%
strings.js — Interp: 🟢 11 · avg +9.0% · Bytecode: 🟢 8, 3 unch. · avg +5.2%
typed-arrays.js — Interp: 🟢 9, 🔴 5, 8 unch. · avg +1.3% · Bytecode: 🟢 13, 🔴 3, 6 unch. · avg +3.0%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
units/Goccia.Values.Iterator.Concat.pas (1)
67-68: Use the repo’s ES spec annotation format consistently.These comments are helpful, but the project requires
// ESYYYY §X.Y.Z SpecMethodName(...)immediately above spec-driven implementations. Please convert theseTC39 Iterator Sequencingcomments and add the missing annotations for the other spec-facing methods in this unit. As per coding guidelines, "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using format// ESYYYY §X.Y.Z SpecMethodName(specParams)."Also applies to: 107-108, 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Iterator.Concat.pas` around lines 67 - 68, Replace the informal "TC39 Iterator Sequencing" comments with the project's ES spec annotation format directly above each spec-driven implementation: use lines like "// ES2020 §7.4.XX IteratorSequencing(OpenNextIterator)" (choose the correct ES year and section number for the referenced behavior) immediately above TGocciaConcatIteratorValue.OpenNextIterator; likewise add the same ESYYYY §X.Y.Z SpecMethodName(params) style annotations above the other spec-facing methods in this unit (the methods around the other reported spots, e.g., the functions at the other comment locations and any iterator-related methods) so every ECMAScript-implemented function (e.g., TGocciaConcatIteratorValue.OpenNextIterator and the other iterator methods) has a single ES spec comment with the proper section and method signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Values.Iterator.Concat.pas`:
- Around line 137-142: When OpenNextIterator can raise, ensure the concat helper
is closed the same way as when the active inner iterator throws: in DirectNext,
wrap the OpenNextIterator call so that if it raises you set FDone := True before
re-raising (or returning the abrupt CreateIteratorResult), mirroring the
existing pattern used when the inner iterator throws; update the logic around
OpenNextIterator and the DirectNext method to set FDone and
return/CreateIteratorResult(TGocciaUndefinedLiteralValue.UndefinedValue, True)
on that error path so subsequent next() calls do not retry the failing iterable.
- Around line 84-88: The code currently hard-casts
FIterables[FCurrentIndex].IteratorMethod to TGocciaFunctionBase and calls .Call,
which fails for non-TGocciaFunctionBase callables; instead invoke the iterator
through the generic callable invocation path used elsewhere: keep creating
TGocciaArgumentsCollection, then obtain the callable from
FIterables[FCurrentIndex].IteratorMethod (respecting its IsCallable contract)
and call it via the common generic call helper/function used in other files (the
same mechanism that accepts any callable value), passing CallArgs and
FIterables[FCurrentIndex].Iterable; update the call site in Iterator.concat to
use that generic-call helper rather than TGocciaFunctionBase(...) so all
callable types are supported.
---
Nitpick comments:
In `@units/Goccia.Values.Iterator.Concat.pas`:
- Around line 67-68: Replace the informal "TC39 Iterator Sequencing" comments
with the project's ES spec annotation format directly above each spec-driven
implementation: use lines like "// ES2020 §7.4.XX
IteratorSequencing(OpenNextIterator)" (choose the correct ES year and section
number for the referenced behavior) immediately above
TGocciaConcatIteratorValue.OpenNextIterator; likewise add the same ESYYYY §X.Y.Z
SpecMethodName(params) style annotations above the other spec-facing methods in
this unit (the methods around the other reported spots, e.g., the functions at
the other comment locations and any iterator-related methods) so every
ECMAScript-implemented function (e.g.,
TGocciaConcatIteratorValue.OpenNextIterator and the other iterator methods) has
a single ES spec comment with the proper section and method signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f280b6d-6d45-47d5-b3d4-d0d8c0c03afa
📒 Files selected for processing (1)
units/Goccia.Values.Iterator.Concat.pas
- Update Iterator.concat spec annotations - Mark concat iterators done when opening the next iterator fails - Preserve failure cleanup in both next variants
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
1 participant
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.
Summary
Iterator.concat(...items)as a new static iterator helper for lazy concatenation of multiple iterables.next()support.Testing
./build.pas testrunner && ./build/TestRunner tests./build/TestRunner tests/built-ins/Iterator/concat.js./build/BenchmarkRunner benchmarks/iterators.js