Add stucturedClone#24
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new global function Changes
Sequence Diagram(s)sequenceDiagram
participant Script
participant Globals
participant Cloner
participant MemoryMap
Script->>Globals: call structuredClone(value)
Globals->>Cloner: StructuredCloneValue(value, MemoryMap)
Cloner->>MemoryMap: check existing mapping
alt mapping exists
MemoryMap-->>Cloner: return existing clone
else mapping not found
Cloner->>Cloner: clone based on type (Object/Array/Map/Set/Primitive)
Cloner->>MemoryMap: record original -> clone
opt composite elements
Cloner->>Cloner: recursively StructuredCloneValue(child, MemoryMap)
end
Cloner-->>Globals: return cloned value
end
Globals-->>Script: return cloned value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Builtins.Globals.pas`:
- Line 47: The file imports the unused unit Goccia.CallStack which causes
compilation failure; remove Goccia.CallStack from the uses clause in
units/Goccia.Builtins.Globals.pas (ensure there are no references to
Goccia.CallStack elsewhere in that unit), recompile, and if any functionality
from Goccia.CallStack is actually required, replace the import with the correct
unit or add the necessary code to reference it (search for usages of CallStack
or symbols from Goccia.CallStack to verify).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
units/Goccia.Builtins.Globals.pas (1)
434-438: Consider GC temp root protection for intermediate cloned values.During structured cloning, newly created values are held only in Pascal code (the
Memorydictionary) until the final result is returned. If GC runs mid-clone, these intermediate values could potentially be collected.Based on learnings: "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."
♻️ Suggested pattern
function TGocciaGlobals.StructuredCloneCallback(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Memory: TDictionary<TGocciaValue, TGocciaValue>; + ClonedValue: TGocciaValue; begin if AArgs.Length = 0 then ThrowTypeError('Failed to execute ''structuredClone'': 1 argument required, but only 0 present.'); Memory := TDictionary<TGocciaValue, TGocciaValue>.Create; try - Result := StructuredCloneValue(AArgs.GetElement(0), Memory); + Result := StructuredCloneValue(AArgs.GetElement(0), Memory); + { Protect all cloned values until returned } + for ClonedValue in Memory.Values do + if Assigned(TGocciaGarbageCollector.Instance) then + TGocciaGarbageCollector.Instance.AddTempRoot(ClonedValue); finally + for ClonedValue in Memory.Values do + if Assigned(TGocciaGarbageCollector.Instance) then + TGocciaGarbageCollector.Instance.RemoveTempRoot(ClonedValue); Memory.Free; end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Globals.pas` around lines 434 - 438, The intermediate cloned values stored only in the Pascal-side Memory dictionary can be collected by the GC; update the cloning logic so every TGocciaValue instance created and placed into Memory is registered with AddTempRoot and later unregistered with RemoveTempRoot when that entry is discarded, and ensure the final value returned from StructuredCloneValue (the Result) is AddTempRoot-protected before Memory.Free and only RemoveTempRoot when the engine no longer needs it; apply these changes inside StructuredCloneValue and around the Memory/Result use to call AddTempRoot/RemoveTempRoot for each value stored in Memory and for Result.
🤖 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.Builtins.Globals.pas`:
- Line 324: The TStringList returned by AObj.GetOwnPropertyKeys is assigned to
Keys but never freed, causing a memory leak; wrap the use of Keys in a
try..finally (or ensure Keys.Free is called) so the list is freed after
processing — locate the assignment to Keys (Keys := AObj.GetOwnPropertyKeys) and
surrounding code in the routine, and change it to obtain Keys, use it, and then
call Keys.Free in a finally block to ensure proper cleanup.
- Around line 337-342: The code currently copies accessor descriptors
(TGocciaPropertyDescriptorAccessor) into the cloned object (via
Result.DefineProperty), but per the structured clone algorithm accessor
properties must cause a DataCloneError because their getter/setter functions are
non-serializable; change the branch that handles
TGocciaPropertyDescriptorAccessor (and any code path that would call
TGocciaPropertyDescriptorAccessor.Create) to raise/throw a DataCloneError
instead of defining the property (use the project's DataCloneError mechanism or
throw an appropriate EDataCloneError), referencing
TGocciaPropertyDescriptorAccessor, Descriptor, Keys[I], and
Result.DefineProperty to locate the code to modify.
---
Duplicate comments:
In `@units/Goccia.Builtins.Globals.pas`:
- Line 47: Remove the unused unit import Goccia.CallStack from the uses clause
in the unit that declares the global builtins (the reference to Goccia.CallStack
is never used), update the uses list so only referenced units remain, and
recompile to ensure the compilation error is resolved; specifically edit the
uses clause where Goccia.CallStack is listed and delete that identifier.
---
Nitpick comments:
In `@units/Goccia.Builtins.Globals.pas`:
- Around line 434-438: The intermediate cloned values stored only in the
Pascal-side Memory dictionary can be collected by the GC; update the cloning
logic so every TGocciaValue instance created and placed into Memory is
registered with AddTempRoot and later unregistered with RemoveTempRoot when that
entry is discarded, and ensure the final value returned from
StructuredCloneValue (the Result) is AddTempRoot-protected before Memory.Free
and only RemoveTempRoot when the engine no longer needs it; apply these changes
inside StructuredCloneValue and around the Memory/Result use to call
AddTempRoot/RemoveTempRoot for each value stored in Memory and for Result.
| Result := TGocciaObjectValue.Create; | ||
| AMemory.Add(AObj, Result); | ||
|
|
||
| Keys := AObj.GetOwnPropertyKeys; |
Contributor
There was a problem hiding this comment.
Memory leak: Keys TStringList is never freed.
GetOwnPropertyKeys returns a TStringList that must be freed after use.
🐛 Proposed fix
function CloneObject(const AObj: TGocciaObjectValue;
const AMemory: TDictionary<TGocciaValue, TGocciaValue>): TGocciaObjectValue;
var
I: Integer;
Keys: TStringList;
Descriptor: TGocciaPropertyDescriptor;
ClonedValue: TGocciaValue;
begin
Result := TGocciaObjectValue.Create;
AMemory.Add(AObj, Result);
Keys := AObj.GetOwnPropertyKeys;
+ try
for I := 0 to Keys.Count - 1 do
begin
Descriptor := AObj.GetOwnPropertyDescriptor(Keys[I]);
if not Assigned(Descriptor) then
Continue;
if Descriptor is TGocciaPropertyDescriptorData then
begin
ClonedValue := StructuredCloneValue(TGocciaPropertyDescriptorData(Descriptor).Value, AMemory);
Result.DefineProperty(Keys[I],
TGocciaPropertyDescriptorData.Create(ClonedValue, Descriptor.Flags));
end
else if Descriptor is TGocciaPropertyDescriptorAccessor then
Result.DefineProperty(Keys[I],
TGocciaPropertyDescriptorAccessor.Create(
TGocciaPropertyDescriptorAccessor(Descriptor).Getter,
TGocciaPropertyDescriptorAccessor(Descriptor).Setter,
Descriptor.Flags));
end;
+ finally
+ Keys.Free;
+ end;
end;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Keys := AObj.GetOwnPropertyKeys; | |
| function CloneObject(const AObj: TGocciaObjectValue; | |
| const AMemory: TDictionary<TGocciaValue, TGocciaValue>): TGocciaObjectValue; | |
| var | |
| I: Integer; | |
| Keys: TStringList; | |
| Descriptor: TGocciaPropertyDescriptor; | |
| ClonedValue: TGocciaValue; | |
| begin | |
| Result := TGocciaObjectValue.Create; | |
| AMemory.Add(AObj, Result); | |
| Keys := AObj.GetOwnPropertyKeys; | |
| try | |
| for I := 0 to Keys.Count - 1 do | |
| begin | |
| Descriptor := AObj.GetOwnPropertyDescriptor(Keys[I]); | |
| if not Assigned(Descriptor) then | |
| Continue; | |
| if Descriptor is TGocciaPropertyDescriptorData then | |
| begin | |
| ClonedValue := StructuredCloneValue(TGocciaPropertyDescriptorData(Descriptor).Value, AMemory); | |
| Result.DefineProperty(Keys[I], | |
| TGocciaPropertyDescriptorData.Create(ClonedValue, Descriptor.Flags)); | |
| end | |
| else if Descriptor is TGocciaPropertyDescriptorAccessor then | |
| Result.DefineProperty(Keys[I], | |
| TGocciaPropertyDescriptorAccessor.Create( | |
| TGocciaPropertyDescriptorAccessor(Descriptor).Getter, | |
| TGocciaPropertyDescriptorAccessor(Descriptor).Setter, | |
| Descriptor.Flags)); | |
| end; | |
| finally | |
| Keys.Free; | |
| end; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` at line 324, The TStringList returned by
AObj.GetOwnPropertyKeys is assigned to Keys but never freed, causing a memory
leak; wrap the use of Keys in a try..finally (or ensure Keys.Free is called) so
the list is freed after processing — locate the assignment to Keys (Keys :=
AObj.GetOwnPropertyKeys) and surrounding code in the routine, and change it to
obtain Keys, use it, and then call Keys.Free in a finally block to ensure proper
cleanup.
Comment on lines
+337
to
+342
| else if Descriptor is TGocciaPropertyDescriptorAccessor then | ||
| Result.DefineProperty(Keys[I], | ||
| TGocciaPropertyDescriptorAccessor.Create( | ||
| TGocciaPropertyDescriptorAccessor(Descriptor).Getter, | ||
| TGocciaPropertyDescriptorAccessor(Descriptor).Setter, | ||
| Descriptor.Flags)); |
Contributor
There was a problem hiding this comment.
Accessor properties should throw, not be copied.
Per the structured clone algorithm, accessor properties contain functions (getter/setter), and functions are non-serializable. Copying them directly violates the spec and could lead to unexpected behavior where cloned objects share function references with the original.
🐛 Proposed fix: throw DataCloneError for accessor properties
if Descriptor is TGocciaPropertyDescriptorData then
begin
ClonedValue := StructuredCloneValue(TGocciaPropertyDescriptorData(Descriptor).Value, AMemory);
Result.DefineProperty(Keys[I],
TGocciaPropertyDescriptorData.Create(ClonedValue, Descriptor.Flags));
end
- else if Descriptor is TGocciaPropertyDescriptorAccessor then
- Result.DefineProperty(Keys[I],
- TGocciaPropertyDescriptorAccessor.Create(
- TGocciaPropertyDescriptorAccessor(Descriptor).Getter,
- TGocciaPropertyDescriptorAccessor(Descriptor).Setter,
- Descriptor.Flags));
+ else if Descriptor is TGocciaPropertyDescriptorAccessor then
+ ThrowTypeError('Failed to execute ''structuredClone'': accessor property could not be cloned.');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| else if Descriptor is TGocciaPropertyDescriptorAccessor then | |
| Result.DefineProperty(Keys[I], | |
| TGocciaPropertyDescriptorAccessor.Create( | |
| TGocciaPropertyDescriptorAccessor(Descriptor).Getter, | |
| TGocciaPropertyDescriptorAccessor(Descriptor).Setter, | |
| Descriptor.Flags)); | |
| else if Descriptor is TGocciaPropertyDescriptorAccessor then | |
| ThrowTypeError('Failed to execute ''structuredClone'': accessor property could not be cloned.'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` around lines 337 - 342, The code currently
copies accessor descriptors (TGocciaPropertyDescriptorAccessor) into the cloned
object (via Result.DefineProperty), but per the structured clone algorithm
accessor properties must cause a DataCloneError because their getter/setter
functions are non-serializable; change the branch that handles
TGocciaPropertyDescriptorAccessor (and any code path that would call
TGocciaPropertyDescriptorAccessor.Create) to raise/throw a DataCloneError
instead of defining the property (use the project's DataCloneError mechanism or
throw an appropriate EDataCloneError), referencing
TGocciaPropertyDescriptorAccessor, Descriptor, Keys[I], and
Result.DefineProperty to locate the code to modify.
Contributor
Benchmark Results179 benchmarks · 🔴 1 regressed · 178 unchanged · avg -1.1% arrays.js — 19 unchanged · avg -0.9%
classes.js — 15 unchanged · avg -0.5%
closures.js — 11 unchanged · avg -2.7%
collections.js — 12 unchanged · avg -3.8%
destructuring.js — 14 unchanged · avg -2.1%
fibonacci.js — 6 unchanged · avg -1.1%
iterators.js — 20 unchanged · avg +1.1%
json.js — 20 unchanged · avg +2.8%
jsx.jsx — 21 unchanged · avg -0.7%
numbers.js — 11 unchanged · avg -1.6%
objects.js — 7 unchanged · avg +0.6%
promises.js — 🔴 1 regressed, 11 unchanged · avg -4.8%
strings.js — 11 unchanged · avg -4.8%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.Builtins.Globals.pas`:
- Around line 423-429: Replace the manual arity check in
TGocciaGlobals.StructuredCloneCallback (the AArgs.Length guard) with the shared
argument validator functions from Goccia.Arguments.Validator: call
RequireAtLeast or RequireExactly (whichever fits the API for structuredClone) at
the start of StructuredCloneCallback to enforce the expected argument count and
produce the standardized error message/behavior; ensure you import/use
Goccia.Arguments.Validator and remove the manual ThrowTypeError branch so the
validator handles the validation.
- Around line 129-130: The binding registers the 'structuredClone' name as a
hardcoded string in AScope.DefineLexicalBinding; replace that literal with the
centralized property-name constant (e.g. add/use a constant like
PropertyNames.StructuredClone in Goccia.Constants.PropertyNames) and use it when
calling AScope.DefineLexicalBinding for the TGocciaNativeFunctionValue created
with StructuredCloneCallback ('structuredClone', 1) so the binding name is
sourced from Goccia.Constants.PropertyNames instead of a raw string.
- Around line 320-389: CloneObject/CloneArray/CloneMap/CloneSet (and the
StructuredCloneValue flow) create new TGocciaValue instances that are only
referenced by locals and the AMemory dictionary during cloning and can be
collected by the GC mid-clone; fix by registering each newly created clone as a
temp root (call AddTempRoot) immediately after creating Result in CloneObject,
CloneArray, CloneMap and CloneSet and ensure you remove those temp roots
(RemoveTempRoot) after cloning completes—preferably in the surrounding
structured-clone call using a try/finally that iterates AMemory.Values to
RemoveTempRoot so all temp roots are cleaned up even on exceptions.
---
Duplicate comments:
In `@units/Goccia.Builtins.Globals.pas`:
- Around line 323-339: GetOwnPropertyKeys returns a TStringList that is not
freed, causing a memory leak; wrap the call to AObj.GetOwnPropertyKeys in a
try..finally and free Keys after the loop (use Keys.Free or FreeAndNil) so Keys
is released even on exceptions—i.e. after obtaining Keys and before exiting the
method, ensure Keys is freed around the for I := 0 to Keys.Count - 1 ... end
block (referencing Keys, GetOwnPropertyKeys, and the loop handling
Descriptor/StructuredCloneValue).
Comment on lines
+129
to
+130
| AScope.DefineLexicalBinding('structuredClone', | ||
| TGocciaNativeFunctionValue.Create(StructuredCloneCallback, 'structuredClone', 1), dtConst); |
Contributor
There was a problem hiding this comment.
Avoid hardcoded structuredClone binding name.
Use a property-name constant (add one if missing) so the global binding name is centralized and consistent. As per coding guidelines: Use runtime constants from split constant units instead of hardcoded string literals: Goccia.Constants.PropertyNames for property names, Goccia.Constants.TypeNames for type names, Goccia.Constants.ErrorNames for error names, Goccia.Constants.ConstructorNames for constructor names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` around lines 129 - 130, The binding
registers the 'structuredClone' name as a hardcoded string in
AScope.DefineLexicalBinding; replace that literal with the centralized
property-name constant (e.g. add/use a constant like
PropertyNames.StructuredClone in Goccia.Constants.PropertyNames) and use it when
calling AScope.DefineLexicalBinding for the TGocciaNativeFunctionValue created
with StructuredCloneCallback ('structuredClone', 1) so the binding name is
sourced from Goccia.Constants.PropertyNames instead of a raw string.
Comment on lines
+320
to
+389
| Result := TGocciaObjectValue.Create; | ||
| AMemory.Add(AObj, Result); | ||
|
|
||
| Keys := AObj.GetOwnPropertyKeys; | ||
| for I := 0 to Keys.Count - 1 do | ||
| begin | ||
| Descriptor := AObj.GetOwnPropertyDescriptor(Keys[I]); | ||
| if not Assigned(Descriptor) then | ||
| Continue; | ||
|
|
||
| if Descriptor is TGocciaPropertyDescriptorData then | ||
| begin | ||
| ClonedValue := StructuredCloneValue(TGocciaPropertyDescriptorData(Descriptor).Value, AMemory); | ||
| Result.DefineProperty(Keys[I], | ||
| TGocciaPropertyDescriptorData.Create(ClonedValue, Descriptor.Flags)); | ||
| end | ||
| else if Descriptor is TGocciaPropertyDescriptorAccessor then | ||
| ThrowTypeError('Failed to execute ''structuredClone'': accessor property ''' + | ||
| Keys[I] + ''' could not be cloned.'); | ||
| end; | ||
| end; | ||
|
|
||
| function CloneArray(const AArr: TGocciaArrayValue; | ||
| const AMemory: TDictionary<TGocciaValue, TGocciaValue>): TGocciaArrayValue; | ||
| var | ||
| I: Integer; | ||
| Element: TGocciaValue; | ||
| begin | ||
| Result := TGocciaArrayValue.Create; | ||
| AMemory.Add(AArr, Result); | ||
|
|
||
| for I := 0 to AArr.Elements.Count - 1 do | ||
| begin | ||
| Element := AArr.Elements[I]; | ||
| if Element = nil then | ||
| Result.Elements.Add(nil) | ||
| else | ||
| Result.Elements.Add(StructuredCloneValue(Element, AMemory)); | ||
| end; | ||
| end; | ||
|
|
||
| function CloneMap(const AMap: TGocciaMapValue; | ||
| const AMemory: TDictionary<TGocciaValue, TGocciaValue>): TGocciaMapValue; | ||
| var | ||
| I: Integer; | ||
| Entry: TGocciaMapEntry; | ||
| begin | ||
| Result := TGocciaMapValue.Create; | ||
| AMemory.Add(AMap, Result); | ||
|
|
||
| for I := 0 to AMap.Entries.Count - 1 do | ||
| begin | ||
| Entry := AMap.Entries[I]; | ||
| Result.SetEntry( | ||
| StructuredCloneValue(Entry.Key, AMemory), | ||
| StructuredCloneValue(Entry.Value, AMemory)); | ||
| end; | ||
| end; | ||
|
|
||
| function CloneSet(const ASet: TGocciaSetValue; | ||
| const AMemory: TDictionary<TGocciaValue, TGocciaValue>): TGocciaSetValue; | ||
| var | ||
| I: Integer; | ||
| begin | ||
| Result := TGocciaSetValue.Create; | ||
| AMemory.Add(ASet, Result); | ||
|
|
||
| for I := 0 to ASet.Items.Count - 1 do | ||
| Result.AddItem(StructuredCloneValue(ASet.Items[I], AMemory)); | ||
| end; |
Contributor
There was a problem hiding this comment.
Temp-root cloned values while they live only in Memory.
CloneObject/CloneArray/CloneMap/CloneSet create new TGocciaValue instances that are only referenced by locals and the Memory dictionary until cloning completes. If GC runs mid-clone, these values can be collected. Add temp roots when creating clones and remove them after the callback returns (e.g., iterate Memory.Values in the finally). Based on learnings: Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` around lines 320 - 389,
CloneObject/CloneArray/CloneMap/CloneSet (and the StructuredCloneValue flow)
create new TGocciaValue instances that are only referenced by locals and the
AMemory dictionary during cloning and can be collected by the GC mid-clone; fix
by registering each newly created clone as a temp root (call AddTempRoot)
immediately after creating Result in CloneObject, CloneArray, CloneMap and
CloneSet and ensure you remove those temp roots (RemoveTempRoot) after cloning
completes—preferably in the surrounding structured-clone call using a
try/finally that iterates AMemory.Values to RemoveTempRoot so all temp roots are
cleaned up even on exceptions.
Comment on lines
+423
to
+429
| function TGocciaGlobals.StructuredCloneCallback(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Memory: TDictionary<TGocciaValue, TGocciaValue>; | ||
| begin | ||
| if AArgs.Length = 0 then | ||
| ThrowTypeError('Failed to execute ''structuredClone'': 1 argument required, but only 0 present.'); | ||
|
|
Contributor
There was a problem hiding this comment.
Use the shared argument validator for the arity check.
Replace the manual AArgs.Length guard with RequireAtLeast/RequireExactly from Goccia.Arguments.Validator for consistent behavior and error messages. As per coding guidelines: Use Goccia.Arguments.Validator.pas for standardized argument count/type validation with RequireExactly and RequireAtLeast instead of manual validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` around lines 423 - 429, Replace the manual
arity check in TGocciaGlobals.StructuredCloneCallback (the AArgs.Length guard)
with the shared argument validator functions from Goccia.Arguments.Validator:
call RequireAtLeast or RequireExactly (whichever fits the API for
structuredClone) at the start of StructuredCloneCallback to enforce the expected
argument count and produce the standardized error message/behavior; ensure you
import/use Goccia.Arguments.Validator and remove the manual ThrowTypeError
branch so the validator handles the validation.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@units/Goccia.Builtins.Globals.pas`:
- Around line 344-391: In CloneArray, CloneMap, and CloneSet register each newly
created clone with AddTempRoot immediately after creating Result (e.g., after
Result := TGocciaArrayValue.Create / TGocciaMapValue.Create /
TGocciaSetValue.Create) so they won’t be collected during cloning, and ensure
these temp roots are removed in the finally block of StructuredCloneCallback;
update StructuredCloneCallback to clean up the AddTempRoot registrations for
these clones when cloning completes or on error.
- Around line 131-132: Replace the hardcoded binding name 'structuredClone' in
the AScope.DefineLexicalBinding call with the relevant constant from
Goccia.Constants.PropertyNames (e.g., PROP_STRUCTURED_CLONE); modify the call
that currently reads AScope.DefineLexicalBinding('structuredClone',
TGocciaNativeFunctionValue.Create(StructuredCloneCallback, 'structuredClone',
1), dtConst) so both occurrences of the literal are replaced by the constant,
and add the appropriate unit to the uses clause if required so
PROP_STRUCTURED_CLONE is in scope.
- Around line 325-341: GetOwnPropertyKeys returns a TStringList (Keys) that is
not freed, causing a memory leak; wrap the usage of Keys in a try..finally and
call Keys.Free in the finally block (around the loop that inspects Descriptor,
calls StructuredCloneValue, and calls Result.DefineProperty), leaving the
existing logic for TGocciaPropertyDescriptorData and
TGocciaPropertyDescriptorAccessor unchanged so Keys is always freed even on
exceptions.
- Around line 429-430: Replace the manual arity guard in the structuredClone
implementation (the AArgs.Length = 0 check that calls ThrowTypeError) with the
shared validator: call RequireAtLeast(AArgs, 1, 'structuredClone') from
Goccia.Arguments.Validator at the start of the structuredClone function and
remove the existing AArgs.Length check and ThrowTypeError call so the common
validator handles the argument validation and error messaging.
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 by CodeRabbit
New Features
Documentation
Tests