Merged
Conversation
- expose compile-time `os` and `arch` on `Goccia.build` - add JS coverage and update built-ins docs
Contributor
📝 WalkthroughWalkthroughAdded new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 277 improved · 🔴 40 regressed · 47 unchanged · avg +5.0% arraybuffer.js — Interp: 🟢 13, 🔴 1 · avg +10.4% · Bytecode: 🟢 2, 🔴 5, 7 unch. · avg -0.4%
arrays.js — Interp: 🟢 17, 2 unch. · avg +7.5% · Bytecode: 🟢 7, 🔴 6, 6 unch. · avg +0.1%
async-await.js — Interp: 🟢 6 · avg +11.7% · Bytecode: 🟢 1, 🔴 2, 3 unch. · avg -0.5%
base64.js — Interp: 🟢 8, 🔴 1, 1 unch. · avg +8.1% · Bytecode: 🟢 3, 🔴 4, 3 unch. · avg -0.6%
classes.js — Interp: 🟢 26, 🔴 2, 3 unch. · avg +5.6% · Bytecode: 🟢 8, 🔴 3, 20 unch. · avg +1.3%
closures.js — Interp: 🟢 11 · avg +6.6% · Bytecode: 🔴 7, 4 unch. · avg -2.6%
collections.js — Interp: 🟢 10, 🔴 2 · avg +6.9% · Bytecode: 🟢 2, 🔴 6, 4 unch. · avg -0.9%
destructuring.js — Interp: 🟢 14, 🔴 3, 5 unch. · avg +2.9% · Bytecode: 🟢 7, 🔴 6, 9 unch. · avg +0.2%
fibonacci.js — Interp: 🟢 7, 1 unch. · avg +7.6% · Bytecode: 🟢 1, 🔴 3, 4 unch. · avg -1.5%
float16array.js — Interp: 🟢 12, 🔴 14, 6 unch. · avg -1.4% · Bytecode: 🟢 12, 🔴 6, 14 unch. · avg +0.8%
for-of.js — Interp: 🟢 3, 4 unch. · avg +2.0% · Bytecode: 🟢 1, 🔴 3, 3 unch. · avg -1.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 29, 🔴 7, 6 unch. · avg +3.2% · Bytecode: 🟢 11, 🔴 18, 13 unch. · avg -0.4%
json.js — Interp: 🟢 20 · avg +7.2% · Bytecode: 🟢 8, 🔴 1, 11 unch. · avg +2.7%
jsx.jsx — Interp: 🟢 16, 5 unch. · avg +3.2% · Bytecode: 🟢 11, 🔴 2, 8 unch. · avg +1.6%
modules.js — Interp: 🟢 9 · avg +8.3% · Bytecode: 🟢 1, 🔴 5, 3 unch. · avg -3.4%
numbers.js — Interp: 🟢 11 · avg +11.2% · Bytecode: 🟢 5, 🔴 1, 5 unch. · avg +1.9%
objects.js — Interp: 🟢 3, 🔴 1, 3 unch. · avg +2.7% · Bytecode: 🟢 4, 🔴 1, 2 unch. · avg +3.0%
promises.js — Interp: 🟢 12 · avg +6.5% · Bytecode: 🟢 5, 🔴 2, 5 unch. · avg +1.1%
regexp.js — Interp: 🟢 11 · avg +7.3% · Bytecode: 🟢 10, 🔴 1 · avg +4.6%
strings.js — Interp: 🟢 19 · avg +7.7% · Bytecode: 🟢 5, 🔴 1, 13 unch. · avg +2.0%
typed-arrays.js — Interp: 🟢 12, 🔴 4, 6 unch. · avg +4.0% · Bytecode: 🟢 4, 🔴 2, 16 unch. · avg +0.8%
uint8array-encoding.js — Interp: 🟢 8, 🔴 5, 5 unch. · avg +1.2% · Bytecode: 🟢 4, 🔴 2, 12 unch. · avg +1.1%
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. |
![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
🧹 Nitpick comments (2)
units/Goccia.Runtime.Bootstrap.pas (1)
546-549: UsePropertyNamesconstants for bootstrapbuildkeys too.Line 546, Line 548, and Line 557 add new runtime property names as string literals. Please route them through
Goccia.Constants.PropertyNamesfor consistency with the project’s runtime-constant policy.As per coding guidelines, use runtime constant units instead of hardcoded string literals for runtime property names (
Goccia.Constants.PropertyNames).Also applies to: 557-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Bootstrap.pas` around lines 546 - 549, The code is adding runtime properties using string literals for keys; replace those literals with the centralized constants from Goccia.Constants.PropertyNames. Update the BuildObj.DefineProperty calls that currently pass 'os', 'arch' (and the one at line ~557) to use the appropriate PropertyNames constant identifiers instead (e.g. PropertyNames.Os, PropertyNames.Arch, etc.), keeping the existing TGocciaPropertyDescriptorData.Create and TGocciaStringLiteralValue.Create calls and their values (GetBuildOS, GetBuildArch) unchanged so only the property name argument is switched to the constant.units/Goccia.Engine.pas (1)
734-738: Use property-name constants forGoccia.buildkeys.Line 735, Line 737, and Line 746 introduce new hardcoded runtime property names (
'os','arch','build'). Please wire these throughGoccia.Constants.PropertyNamesto keep the metadata surface centralized and avoid drift across bootstrap paths.♻️ Suggested change
- BuildObj.DefineProperty('os', TGocciaPropertyDescriptorData.Create( + BuildObj.DefineProperty(PROP_OS, TGocciaPropertyDescriptorData.Create( TGocciaStringLiteralValue.Create(GetBuildOS), [pfEnumerable])); - BuildObj.DefineProperty('arch', TGocciaPropertyDescriptorData.Create( + BuildObj.DefineProperty(PROP_ARCH, TGocciaPropertyDescriptorData.Create( TGocciaStringLiteralValue.Create(GetBuildArch), [pfEnumerable])); @@ - GocciaObj.AssignProperty('build', BuildObj); + GocciaObj.AssignProperty(PROP_BUILD, BuildObj);As per coding guidelines, use runtime constant units instead of hardcoded string literals for runtime property names (
Goccia.Constants.PropertyNames).Also applies to: 746-746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Engine.pas` around lines 734 - 738, Replace hardcoded property name literals used when defining the Goccia.build object with the canonical constants from Goccia.Constants.PropertyNames: locate where BuildObj is populated (TGocciaObjectValue.Create and the DefineProperty calls in units/Goccia.Engine.pas), and swap the string keys 'os', 'arch' and the 'build' property usage to use the corresponding constants (e.g., PropertyNames.Os, PropertyNames.Arch, PropertyNames.Build) so all runtime property names are centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/built-ins/Goccia/build.js`:
- Around line 6-7: The test currently gates the entire Goccia.build suite behind
a runtime check (const hasGoccia = typeof Goccia !== "undefined") which can mask
regressions; remove the conditional skip and make the suite unconditional,
replacing any top-level existence check with an explicit assertion inside the
suite (assert typeof Goccia !== "undefined" or equivalent) and keep testing
Goccia.build directly; update references to hasGoccia and any conditional
branches so the tests always run and fail fast if the Goccia global is missing.
---
Nitpick comments:
In `@units/Goccia.Engine.pas`:
- Around line 734-738: Replace hardcoded property name literals used when
defining the Goccia.build object with the canonical constants from
Goccia.Constants.PropertyNames: locate where BuildObj is populated
(TGocciaObjectValue.Create and the DefineProperty calls in
units/Goccia.Engine.pas), and swap the string keys 'os', 'arch' and the 'build'
property usage to use the corresponding constants (e.g., PropertyNames.Os,
PropertyNames.Arch, PropertyNames.Build) so all runtime property names are
centralized and consistent.
In `@units/Goccia.Runtime.Bootstrap.pas`:
- Around line 546-549: The code is adding runtime properties using string
literals for keys; replace those literals with the centralized constants from
Goccia.Constants.PropertyNames. Update the BuildObj.DefineProperty calls that
currently pass 'os', 'arch' (and the one at line ~557) to use the appropriate
PropertyNames constant identifiers instead (e.g. PropertyNames.Os,
PropertyNames.Arch, etc.), keeping the existing
TGocciaPropertyDescriptorData.Create and TGocciaStringLiteralValue.Create calls
and their values (GetBuildOS, GetBuildArch) unchanged so only the property name
argument is switched to the constant.
🪄 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: 4d415694-f75c-4224-b85d-c63a49c306d9
📒 Files selected for processing (6)
AGENTS.mddocs/built-ins.mdtests/built-ins/Goccia/build.jsunits/Goccia.Engine.pasunits/Goccia.Platform.pasunits/Goccia.Runtime.Bootstrap.pas
frostney
added a commit
that referenced
this pull request
Apr 11, 2026
Incorporates 5 commits from main: - TextEncoder and TextDecoder built-ins (#272) - Make import.meta tests account for Windows drive letters (#274) - Fixes tagged template object identity (#275) - Add Goccia.build platform metadata (#276) - Add ToObject coercion for primitives across all Object.* static methods (#271) Conflict resolution in 3 files (all "both sides added" — keep both): Goccia.Engine.pas / Goccia.Runtime.Bootstrap.pas: - Added ggTextEncoder and ggTextDecoder to TGocciaGlobalBuiltin enum - Added both to DefaultGlobals alongside ggURL - Added FBuiltinTextEncoder/FBuiltinTextDecoder fields, Free calls, registration blocks, Expose* helpers, and constructor TypeDef blocks alongside the existing URL equivalents Goccia.Values.ClassValue.pas: - Added TGocciaTextEncoderClassValue and TGocciaTextDecoderClassValue declarations, impl uses, and CreateNativeInstance implementations alongside the existing URL equivalents Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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
Goccia.buildobject with compile-timeosandarchmetadata.Gocciaglobal.docs/built-ins.mdand add end-to-end coverage intests/built-ins/Goccia/build.js.Testing
Goccia.buildshape, value stability, enumerability, and read-only behavior.Goccia.buildcontract and supported platform values.