Conversation
Contributor
📝 WalkthroughWalkthroughAdds a generic insertion-ordered map (TOrderedMap) and refactors scope binding and object property storage to use it, replacing several custom map/list patterns and updating related iteration and property-access code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Benchmark Results179 benchmarks · 🟢 142 improved · 37 unchanged · avg +20.8% arrays.js — 🟢 5 improved, 14 unchanged · avg +3.2%
classes.js — 🟢 15 improved · avg +20.1%
closures.js — 🟢 10 improved, 1 unchanged · avg +14.4%
collections.js — 🟢 9 improved, 3 unchanged · avg +23.3%
destructuring.js — 🟢 14 improved · avg +26.3%
fibonacci.js — 🟢 4 improved, 2 unchanged · avg +28.5%
iterators.js — 🟢 20 improved · avg +52.0%
json.js — 🟢 19 improved, 1 unchanged · avg +21.3%
jsx.jsx — 🟢 21 improved · avg +17.0%
numbers.js — 🟢 4 improved, 7 unchanged · avg +4.9%
objects.js — 🟢 7 improved · avg +36.8%
promises.js — 🟢 11 improved, 1 unchanged · avg +15.7%
strings.js — 🟢 3 improved, 8 unchanged · avg +5.7%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Values.ObjectValue.pas (1)
115-132:⚠️ Potential issue | 🟠 MajorFree old descriptors before replacement in
DefineProperty,Freeze, andSeal.
TOrderedMapdoes not own its values. WhenFProperties.Add()overwrites an existing key, the oldTGocciaPropertyDescriptorinstance is not freed. Since descriptor classes hold no destructors, their containedTGocciaValuereferences (inTGocciaPropertyDescriptorData.FValueandTGocciaPropertyDescriptorAccessor.FGetter/FSetter) also leak.This occurs in:
DefineProperty(line 316): replaces existing descriptor without freeingFreeze(lines 659, 706): creates new descriptors for all properties, discards old onesSeal(line 705): same pattern as FreezeRetrieve and free the old descriptor before calling
Add(), or add destructors to descriptor classes to handle cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 115 - 132, DefineProperty, Freeze, and Seal currently overwrite TGocciaPropertyDescriptor instances without freeing the old ones; before calling FProperties.Add (and before replacing entries in FSymbolDescriptors), retrieve the existing descriptor (e.g., via TryGetValue/GetValue or the map's lookup method), Free it, then add the new descriptor; alternatively implement destructors on TGocciaPropertyDescriptor / TGocciaPropertyDescriptorData / TGocciaPropertyDescriptorAccessor to release held TGocciaValue references and call Free on old descriptor whenever it would be replaced (apply this change in the DefineProperty, Freeze and Seal code paths where new descriptors replace existing ones).
🧹 Nitpick comments (1)
units/OrderedMap.pas (1)
53-80: Preferconstfor non-mutated parameters.
AHash,ANewBucketCount, andAIndexare not modified and should beconstfor consistency and clarity.As per coding guidelines: “Prefer `const` parameters for all parameters that are not modified in the function body.”♻️ Proposed updates
- function FindBucket(const AKey: string; AHash: Cardinal; out ABucketIdx: Integer): Boolean; + function FindBucket(const AKey: string; const AHash: Cardinal; out ABucketIdx: Integer): Boolean; ... - procedure Rehash(ANewBucketCount: Integer); + procedure Rehash(const ANewBucketCount: Integer); ... - function EntryAt(AIndex: Integer): TKeyValuePair; + function EntryAt(const AIndex: Integer): TKeyValuePair;-function TOrderedMap<TValue>.FindBucket(const AKey: string; AHash: Cardinal; +function TOrderedMap<TValue>.FindBucket(const AKey: string; const AHash: Cardinal; out ABucketIdx: Integer): Boolean; ... -procedure TOrderedMap<TValue>.Rehash(ANewBucketCount: Integer); +procedure TOrderedMap<TValue>.Rehash(const ANewBucketCount: Integer); ... -function TOrderedMap<TValue>.EntryAt(AIndex: Integer): TKeyValuePair; +function TOrderedMap<TValue>.EntryAt(const AIndex: Integer): TKeyValuePair;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/OrderedMap.pas` around lines 53 - 80, Parameters that are never modified should be declared const: update the signatures for FindBucket to make AHash const, Rehash to make ANewBucketCount const, and EntryAt to make AIndex const so they reflect immutability and follow the project's guideline; locate the methods by name (FindBucket, Rehash, EntryAt) in OrderedMap.pas and change those parameter declarations to use const, then rebuild to ensure no call sites require non-const parameters.
🤖 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/OrderedMap.pas`:
- Around line 19-21: Rename the TOrderedMap generic class to TGocciaOrderedMap
to follow the TGoccia<Name> class naming convention: update the type declaration
(TOrderedMap<TValue> -> TGocciaOrderedMap<TValue>), adjust any
constructor/destructor and class references (uses, variable declarations,
typecasts, and unit exports) that reference TOrderedMap, and ensure any
unit/interface/implementation identifiers that expose the class are updated to
the new TGocciaOrderedMap symbol so all references compile.
---
Outside diff comments:
In `@units/Goccia.Values.ObjectValue.pas`:
- Around line 115-132: DefineProperty, Freeze, and Seal currently overwrite
TGocciaPropertyDescriptor instances without freeing the old ones; before calling
FProperties.Add (and before replacing entries in FSymbolDescriptors), retrieve
the existing descriptor (e.g., via TryGetValue/GetValue or the map's lookup
method), Free it, then add the new descriptor; alternatively implement
destructors on TGocciaPropertyDescriptor / TGocciaPropertyDescriptorData /
TGocciaPropertyDescriptorAccessor to release held TGocciaValue references and
call Free on old descriptor whenever it would be replaced (apply this change in
the DefineProperty, Freeze and Seal code paths where new descriptors replace
existing ones).
---
Nitpick comments:
In `@units/OrderedMap.pas`:
- Around line 53-80: Parameters that are never modified should be declared
const: update the signatures for FindBucket to make AHash const, Rehash to make
ANewBucketCount const, and EntryAt to make AIndex const so they reflect
immutability and follow the project's guideline; locate the methods by name
(FindBucket, Rehash, EntryAt) in OrderedMap.pas and change those parameter
declarations to use const, then rebuild to ensure no call sites require
non-const parameters.
Comment on lines
+19
to
+21
| type | ||
| TOrderedMap<TValue> = class | ||
| public type |
Contributor
There was a problem hiding this comment.
Rename TOrderedMap to TGocciaOrderedMap to match naming conventions.
The class name doesn’t follow the required TGoccia<Name> prefix convention.
As per coding guidelines: “Class naming: Use TGoccia<Name> prefix for all class names.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/OrderedMap.pas` around lines 19 - 21, Rename the TOrderedMap generic
class to TGocciaOrderedMap to follow the TGoccia<Name> class naming convention:
update the type declaration (TOrderedMap<TValue> -> TGocciaOrderedMap<TValue>),
adjust any constructor/destructor and class references (uses, variable
declarations, typecasts, and unit exports) that reference TOrderedMap, and
ensure any unit/interface/implementation identifiers that expose the class are
updated to the new TGocciaOrderedMap symbol so all references compile.
frostney
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
units/Goccia.Values.ObjectValue.pas (2)
332-340: Identical implementations forGetOwnPropertyNamesandGetOwnPropertyKeys.Both methods return
FProperties.Keys. If they serve distinct semantic purposes (e.g., future symbol key support forGetOwnPropertyKeys), this is fine. Otherwise, one could delegate to the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 332 - 340, TGocciaObjectValue.GetOwnPropertyNames and TGocciaObjectValue.GetOwnPropertyKeys both return FProperties.Keys; if they are intended to be semantically identical, make one delegate to the other to avoid duplicate logic (e.g., have GetOwnPropertyKeys call GetOwnPropertyNames or vice versa), otherwise implement the intended difference (for example, change GetOwnPropertyKeys to include symbol keys or a different source than FProperties.Keys). Update the implementations of GetOwnPropertyNames and GetOwnPropertyKeys accordingly, keeping FProperties.Keys as the canonical source unless symbol/key differences are required.
544-558: Consistency note: Symbol properties still use dictionary pattern.String properties now use
TGocciaPropertyMapwhile symbol properties retainTDictionary+TListfor insertion order. Consider aligning these implementations in a follow-up for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 544 - 558, The symbol property storage is inconsistent: replace the TDictionary + TList pattern (FSymbolDescriptors, FSymbolInsertionOrder) with the same TGocciaPropertyMap used for string properties so insertion order and descriptor management are unified; update TGocciaObjectValue.DefineSymbolProperty to use the TGocciaPropertyMap APIs (lookup/update/remove/iterate) instead of TryGetValue/AddOrSetValue and remove manual insertion-order handling, and adjust any other symbol-related methods to use the new map field and its methods to preserve insertion order and configurable checks.units/OrderedMap.pas (2)
10-10: Unit naming should followGoccia.<Category>.<Name>.pasconvention.The unit is named
OrderedMapbut project conventions require theGoccia.<Category>.<Name>.pasnaming pattern. Consider renaming toGoccia.Collections.OrderedMap.pasor similar.As per coding guidelines: "Unit naming convention:
Goccia.<Category>.<Name>.pas"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/OrderedMap.pas` at line 10, The unit declaration and filename should follow the project's naming convention: rename the unit from "OrderedMap" to a Goccia-style name such as "Goccia.Collections.OrderedMap" and update the filename accordingly (e.g., Goccia.Collections.OrderedMap.pas); then update any references/usages of the unit in uses clauses and project files to the new unit identifier (search for "unit OrderedMap;" and occurrences of "OrderedMap" in uses lists) so compilation and unit resolution remain correct.
395-396: Minor edge case in error message.When
FCountis 0, the error message will display "index 0 out of range [0..-1]", which is slightly confusing. Consider a clearer message for the empty map case.💡 Suggested improvement
if (AIndex < 0) or (AIndex >= FCount) then - raise ERangeError.CreateFmt('EntryAt index %d out of range [0..%d]', [AIndex, FCount - 1]); + if FCount = 0 then + raise ERangeError.Create('EntryAt called on empty map') + else + raise ERangeError.CreateFmt('EntryAt index %d out of range [0..%d]', [AIndex, FCount - 1]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/OrderedMap.pas` around lines 395 - 396, The range-check in the EntryAt (or similar index-access) branch constructs "0..-1" when FCount = 0; update the check to produce a clearer message for empty maps by testing FCount = 0 first and raising ERangeError with a message like "EntryAt index %d out of range: map is empty" (or otherwise omit the upper bound), otherwise keep the existing CreateFmt('EntryAt index %d out of range [0..%d]', [AIndex, FCount - 1]) for FCount > 0 so the error is human-readable in both cases.
🤖 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.ObjectValue.pas`:
- Around line 332-340: TGocciaObjectValue.GetOwnPropertyNames and
TGocciaObjectValue.GetOwnPropertyKeys both return FProperties.Keys; if they are
intended to be semantically identical, make one delegate to the other to avoid
duplicate logic (e.g., have GetOwnPropertyKeys call GetOwnPropertyNames or vice
versa), otherwise implement the intended difference (for example, change
GetOwnPropertyKeys to include symbol keys or a different source than
FProperties.Keys). Update the implementations of GetOwnPropertyNames and
GetOwnPropertyKeys accordingly, keeping FProperties.Keys as the canonical source
unless symbol/key differences are required.
- Around line 544-558: The symbol property storage is inconsistent: replace the
TDictionary + TList pattern (FSymbolDescriptors, FSymbolInsertionOrder) with the
same TGocciaPropertyMap used for string properties so insertion order and
descriptor management are unified; update
TGocciaObjectValue.DefineSymbolProperty to use the TGocciaPropertyMap APIs
(lookup/update/remove/iterate) instead of TryGetValue/AddOrSetValue and remove
manual insertion-order handling, and adjust any other symbol-related methods to
use the new map field and its methods to preserve insertion order and
configurable checks.
In `@units/OrderedMap.pas`:
- Line 10: The unit declaration and filename should follow the project's naming
convention: rename the unit from "OrderedMap" to a Goccia-style name such as
"Goccia.Collections.OrderedMap" and update the filename accordingly (e.g.,
Goccia.Collections.OrderedMap.pas); then update any references/usages of the
unit in uses clauses and project files to the new unit identifier (search for
"unit OrderedMap;" and occurrences of "OrderedMap" in uses lists) so compilation
and unit resolution remain correct.
- Around line 395-396: The range-check in the EntryAt (or similar index-access)
branch constructs "0..-1" when FCount = 0; update the check to produce a clearer
message for empty maps by testing FCount = 0 first and raising ERangeError with
a message like "EntryAt index %d out of range: map is empty" (or otherwise omit
the upper bound), otherwise keep the existing CreateFmt('EntryAt index %d out of
range [0..%d]', [AIndex, FCount - 1]) for FCount > 0 so the error is
human-readable in both cases.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
units/OrderedMap.pas (2)
318-322: Consider using a more specific exception type.
EArgumentExceptionor a customEKeyNotFoundexception would provide clearer diagnostics than the genericExceptionclass.🔧 Suggested improvement
function TOrderedMap<TValue>.GetValue(const AKey: string): TValue; begin if not TryGetValue(AKey, Result) then - raise Exception.Create('Key not found in ordered map'); + raise EArgumentException.CreateFmt('Key ''%s'' not found in ordered map', [AKey]); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/OrderedMap.pas` around lines 318 - 322, Change the generic Exception raised in TOrderedMap<TValue>.GetValue to a more specific exception type; either raise EArgumentException (e.g., using EArgumentException.CreateFmt to include the missing AKey in the message) or define and raise a custom EKeyNotFound = class(Exception) and use EKeyNotFound.CreateFmt to include the key for clearer diagnostics; update the raise statement in GetValue accordingly so callers can catch a specific exception.
391-412: O(n) complexity for index-based access.
EntryAtscans entries linearly to find the nth active element. For sequential iteration, preferForEachorToArraywhich are more efficient. This is fine for occasional single-element access but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/OrderedMap.pas` around lines 391 - 412, Add a short API comment above TOrderedMap<TValue>.EntryAt noting that EntryAt performs a linear scan over FEntries (O(n) time) to find the nth active element and is intended for occasional single-element access; recommend using ForEach or ToArray for sequential iteration or bulk retrieval instead, and mention that complexity stems from scanning FEntryCount/FEntries for Active entries. This guidance should reference the EntryAt method name and the ForEach/ToArray helpers so callers know the alternatives.
🤖 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/OrderedMap.pas`:
- Around line 19-21: Rename the class symbol TOrderedMap to follow the
TGoccia<Name> convention: update the class declaration TOrderedMap<TValue> to
TGocciaOrderedMap<TValue> and update all internal references/usages (type
declarations, variables, method declarations, constructors/destructors, and any
casts) that reference TOrderedMap to use TGocciaOrderedMap instead so the
identifier is consistent across the unit.
---
Nitpick comments:
In `@units/OrderedMap.pas`:
- Around line 318-322: Change the generic Exception raised in
TOrderedMap<TValue>.GetValue to a more specific exception type; either raise
EArgumentException (e.g., using EArgumentException.CreateFmt to include the
missing AKey in the message) or define and raise a custom EKeyNotFound =
class(Exception) and use EKeyNotFound.CreateFmt to include the key for clearer
diagnostics; update the raise statement in GetValue accordingly so callers can
catch a specific exception.
- Around line 391-412: Add a short API comment above TOrderedMap<TValue>.EntryAt
noting that EntryAt performs a linear scan over FEntries (O(n) time) to find the
nth active element and is intended for occasional single-element access;
recommend using ForEach or ToArray for sequential iteration or bulk retrieval
instead, and mention that complexity stems from scanning FEntryCount/FEntries
for Active entries. This guidance should reference the EntryAt method name and
the ForEach/ToArray helpers so callers know the alternatives.
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