LibJS: Make flapc read its layout stuff by parsing C++ headers - #10889
LibJS: Make flapc read its layout stuff by parsing C++ headers#10889alimpfard wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces the C++ interpreter layout generator with a libclang-based flapc pipeline. It adds a layout specification, probe extraction, CLI options, host-tool build support, dependency tracking, retained ChangesInterpreter layout extraction
CI container image updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CMake
participant flapc
participant libclang
participant InterpreterAssembly
CMake->>flapc: build host tool with libclang and clang arguments
CMake->>flapc: pass layout spec and clang-args
flapc->>libclang: parse generated C++ probe
libclang-->>flapc: return layout values and dependencies
flapc->>CMake: write layout.conf and depfile
CMake->>InterpreterAssembly: generate interpreter assembly
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Libraries/LibJS/CMakeLists.txt (1)
362-367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuelibclang is now a hard build requirement.
find_library(... REQUIRED)will fail configuration on machines without libclang, which is a new prerequisite for building LibJS. Worth calling out in the build documentation and CI images.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/CMakeLists.txt` around lines 362 - 367, Document libclang as a required LibJS build prerequisite and update the relevant CI image or setup configuration to install it, reflecting the REQUIRED find_library call in the libclang detection block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibJS/Flap/src/frontend/clang.rs`:
- Around line 68-71: Update the libclang evaluation handling around
clang_Cursor_Evaluate to detect unsigned results with
clang_EvalResult_isUnsignedInt and read them via clang_EvalResult_getAsUnsigned,
while retaining clang_EvalResult_getAsLongLong for signed results. Ensure the
non-hex path preserves the full unsigned value instead of interpreting values
above i64::MAX as negative.
---
Nitpick comments:
In `@Libraries/LibJS/CMakeLists.txt`:
- Around line 362-367: Document libclang as a required LibJS build prerequisite
and update the relevant CI image or setup configuration to install it,
reflecting the REQUIRED find_library call in the libclang detection block.
🪄 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 Plus
Run ID: b047e52f-be30-4abd-b79a-8c8e8ede7e03
📒 Files selected for processing (13)
Libraries/LibJS/Bytecode/Executable.hLibraries/LibJS/CMakeLists.txtLibraries/LibJS/Flap/build.rsLibraries/LibJS/Flap/src/frontend/clang.rsLibraries/LibJS/Flap/src/frontend/layout_source.rsLibraries/LibJS/Flap/src/frontend/mod.rsLibraries/LibJS/Flap/src/lib.rsLibraries/LibJS/Flap/src/main.rsLibraries/LibJS/Interpreter/GenerateLayout.cppLibraries/LibJS/Interpreter/layout.specLibraries/LibJS/Runtime/Object.cppLibraries/LibJS/Runtime/Object.hMeta/CMake/rust_crate.cmake
💤 Files with no reviewable changes (1)
- Libraries/LibJS/Interpreter/GenerateLayout.cpp
github-actions
Bot
removed
the
conflicts
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)
.github/workflows/lint-code.yml (1)
34-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUpdate the Flap job to the new CI image.
The newly added
flapjob still runs in2026.07.04, while the surrounding CI and the PR objective use2026.07.24. This can leave Flap checks without the updated libclang/toolchain dependencies.Proposed fix
- image: ghcr.io/ladybirdbrowser/ladybird-ci:2026.07.04 + image: ghcr.io/ladybirdbrowser/ladybird-ci:2026.07.24🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/lint-code.yml at line 34, Update the image tag used by the Flap job to ghcr.io/ladybirdbrowser/ladybird-ci:2026.07.24, keeping the existing image repository and job configuration unchanged.
🧹 Nitpick comments (4)
Libraries/LibJS/Interpreter/layout.spec (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude
.specfrom Ruby linting.RuboCop is parsing this file as Ruby and emitting
fatalsyntax errors (lines 28 and 72) — a false positive from the.specextension, but a fatal-level one that may fail the lint job. Add an exclusion in.rubocop.ymlforLibraries/LibJS/**/*.spec(or the path glob your config uses).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/Interpreter/layout.spec` at line 1, Update the RuboCop configuration to exclude files matching Libraries/LibJS/**/*.spec from Ruby linting, using the repository’s existing exclusion structure and glob conventions; leave the layout.spec file unchanged.Source: Linters/SAST tools
205-221: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive vector member offsets from the actual member type rather than hardcoding
Vector<Value>.Lines 205-207 and 211-220 compute data/size offsets via
__builtin_offsetof(Vector<Value>, ...)but apply them to members that are notVector<Value>(property_lookup_caches,global_variable_caches,environment_coordinate_caches,m_property_values). That silently assumes everyVector<T>instantiation has identical member offsets — which stops holding the moment any of these gains an inline capacity, and the failure mode is a wrong pointer in generated asm rather than a build error.Lines 315-320 already use the robust form; applying it consistently would make the spec self-checking.
♻️ Example for line 211
-const EXECUTABLE_PROPERTY_LOOKUP_CACHES_DATA = offsetof(Executable, property_lookup_caches) + __builtin_offsetof(Vector<Value>, m_metadata.outline_buffer) +const EXECUTABLE_PROPERTY_LOOKUP_CACHES_DATA = offsetof(Executable, property_lookup_caches) + __builtin_offsetof(decltype(Executable::property_lookup_caches), m_metadata.outline_buffer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/Interpreter/layout.spec` around lines 205 - 221, Update the offset constants for the non-Value vector members to derive offsets from each member’s actual type instead of hardcoding __builtin_offsetof(Vector<Value>, ...). Apply this to Executable property_lookup_caches, global_variable_caches, environment_coordinate_caches, constants, and ObjectPropertyIteratorCacheData::m_property_values, matching the robust pattern already used around lines 315-320; keep the existing offset semantics and raw-field declarations unchanged.
314-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded values bypass header-derived extraction.
Three constants are literals rather than queried from C++:
- Line 314:
1 << 1instead of the corresponding binding-flag enumerator.- Line 339:
BYTE_LENGTH_U32_INDEX = 2.- Line 343:
+ 4, which bakes insizeof(u32)and a little-endian half-word position.Each will drift silently if the underlying C++ changes — the exact failure mode this PR removes elsewhere. Where an enumerator or
sizeof/offsetofexpression exists, prefer it; otherwise a short comment stating the invariant would help.Also applies to: 339-339, 343-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/Interpreter/layout.spec` at line 314, Replace the literals in BINDING_FLAG_MUTABLE, BYTE_LENGTH_U32_INDEX, and the offset calculation near the latter with values extracted from the corresponding C++ binding enumerator and sizeof/offsetof definitions. Reuse the existing header-derived extraction mechanism in this spec, and add a concise invariant comment only where no extractable symbol exists..github/workflows/js-and-wasm-benchmarks.yml (1)
23-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused
ci_containeranchors from the benchmark workflows. Both.github/workflows/js-and-wasm-benchmarks.ymland.github/workflows/web-benchmarks.ymldefine&ci_containerwithout any*ci_containerreference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/js-and-wasm-benchmarks.yml at line 23, Remove the unused ci_container anchor from the container declarations in .github/workflows/js-and-wasm-benchmarks.yml:23-23 and .github/workflows/web-benchmarks.yml:25-25, while preserving each workflow’s container image configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Documentation/BuildInstructionsLadybird.md`:
- Line 217: Update the FreeBSD build instructions around the pkg install command
to configure CMake with the LLVM 19 prefix at /usr/local/llvm19, using
CMAKE_PREFIX_PATH or LLVM_DIR (preferably derived via llvm-config19), so the
existing CMake lookup can locate libclang.
---
Outside diff comments:
In @.github/workflows/lint-code.yml:
- Line 34: Update the image tag used by the Flap job to
ghcr.io/ladybirdbrowser/ladybird-ci:2026.07.24, keeping the existing image
repository and job configuration unchanged.
---
Nitpick comments:
In @.github/workflows/js-and-wasm-benchmarks.yml:
- Line 23: Remove the unused ci_container anchor from the container declarations
in .github/workflows/js-and-wasm-benchmarks.yml:23-23 and
.github/workflows/web-benchmarks.yml:25-25, while preserving each workflow’s
container image configuration.
In `@Libraries/LibJS/Interpreter/layout.spec`:
- Line 1: Update the RuboCop configuration to exclude files matching
Libraries/LibJS/**/*.spec from Ruby linting, using the repository’s existing
exclusion structure and glob conventions; leave the layout.spec file unchanged.
- Around line 205-221: Update the offset constants for the non-Value vector
members to derive offsets from each member’s actual type instead of hardcoding
__builtin_offsetof(Vector<Value>, ...). Apply this to Executable
property_lookup_caches, global_variable_caches, environment_coordinate_caches,
constants, and ObjectPropertyIteratorCacheData::m_property_values, matching the
robust pattern already used around lines 315-320; keep the existing offset
semantics and raw-field declarations unchanged.
- Line 314: Replace the literals in BINDING_FLAG_MUTABLE, BYTE_LENGTH_U32_INDEX,
and the offset calculation near the latter with values extracted from the
corresponding C++ binding enumerator and sizeof/offsetof definitions. Reuse the
existing header-derived extraction mechanism in this spec, and add a concise
invariant comment only where no extractable symbol exists.
🪄 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 Plus
Run ID: b35c3619-f17d-4b15-89b1-2163bd5d1dcd
📒 Files selected for processing (23)
.devcontainer/features/ladybird/install-fedora.sh.devcontainer/features/ladybird/install-ubuntu.sh.github/workflows/ci.yml.github/workflows/js-and-wasm-artifacts.yml.github/workflows/js-and-wasm-benchmarks.yml.github/workflows/libjs-test262.yml.github/workflows/lint-code.yml.github/workflows/nightly-lagom.yml.github/workflows/web-benchmarks.ymlDocumentation/BuildInstructionsLadybird.mdLibraries/LibJS/Bytecode/Executable.hLibraries/LibJS/CMakeLists.txtLibraries/LibJS/Flap/build.rsLibraries/LibJS/Flap/src/frontend/clang.rsLibraries/LibJS/Flap/src/frontend/layout_source.rsLibraries/LibJS/Flap/src/frontend/mod.rsLibraries/LibJS/Flap/src/lib.rsLibraries/LibJS/Flap/src/main.rsLibraries/LibJS/Interpreter/GenerateLayout.cppLibraries/LibJS/Interpreter/layout.specLibraries/LibJS/Runtime/Object.cppLibraries/LibJS/Runtime/Object.hMeta/CMake/rust_crate.cmake
💤 Files with no reviewable changes (1)
- Libraries/LibJS/Interpreter/GenerateLayout.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- Libraries/LibJS/Flap/build.rs
- Libraries/LibJS/Runtime/Object.h
- Libraries/LibJS/Bytecode/Executable.h
- Libraries/LibJS/Flap/src/lib.rs
- Libraries/LibJS/Flap/src/frontend/mod.rs
- Libraries/LibJS/CMakeLists.txt
- Libraries/LibJS/Runtime/Object.cpp
- Libraries/LibJS/Flap/src/main.rs
- Libraries/LibJS/Flap/src/frontend/layout_source.rs
- Meta/CMake/rust_crate.cmake
- Libraries/LibJS/Flap/src/frontend/clang.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Libraries/LibJS/Interpreter/layout.spec (1)
314-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded values that the spec could derive from the headers.
BINDING_FLAG_MUTABLE = 1 << 1,BYTE_LENGTH_U32_INDEX = 2, and the+ 4inTYPED_ARRAY_ARRAY_LENGTH_INDEXsilently drift if the corresponding C++ enum/variant/field types change — exactly the failure mode this spec exists to prevent. Where a C++ expression is available (e.g. the binding-flag enumerator,sizeof(u32)), prefer it.Also applies to: 339-339, 343-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/Interpreter/layout.spec` at line 314, Replace the hardcoded layout constants in the spec—BINDING_FLAG_MUTABLE, BYTE_LENGTH_U32_INDEX, and the +4 used by TYPED_ARRAY_ARRAY_LENGTH_INDEX—with values derived from the corresponding C++ headers or exposed C++ expressions, using the binding-flag enumerator and sizeof(u32) where applicable. Keep the existing layout assertions and index semantics unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/js-and-wasm-benchmarks.yml:
- Line 23: Remove the unused &ci_container YAML anchor from the container
entries in .github/workflows/js-and-wasm-benchmarks.yml lines 23-23 and
.github/workflows/web-benchmarks.yml lines 25-25, leaving the container image
configuration unchanged; do not add an alias.
---
Nitpick comments:
In `@Libraries/LibJS/Interpreter/layout.spec`:
- Line 314: Replace the hardcoded layout constants in the
spec—BINDING_FLAG_MUTABLE, BYTE_LENGTH_U32_INDEX, and the +4 used by
TYPED_ARRAY_ARRAY_LENGTH_INDEX—with values derived from the corresponding C++
headers or exposed C++ expressions, using the binding-flag enumerator and
sizeof(u32) where applicable. Keep the existing layout assertions and index
semantics unchanged.
🪄 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 Plus
Run ID: 5decf6d8-6244-4fd9-9b71-d8f294594bd4
📒 Files selected for processing (20)
.devcontainer/features/ladybird/install-fedora.sh.devcontainer/features/ladybird/install-ubuntu.sh.github/workflows/ci.yml.github/workflows/js-and-wasm-artifacts.yml.github/workflows/js-and-wasm-benchmarks.yml.github/workflows/libjs-test262.yml.github/workflows/lint-code.yml.github/workflows/nightly-lagom.yml.github/workflows/web-benchmarks.ymlDocumentation/BuildInstructionsLadybird.mdLibraries/LibJS/CMakeLists.txtLibraries/LibJS/Flap/build.rsLibraries/LibJS/Flap/src/frontend/clang.rsLibraries/LibJS/Flap/src/frontend/layout_source.rsLibraries/LibJS/Flap/src/frontend/mod.rsLibraries/LibJS/Flap/src/lib.rsLibraries/LibJS/Flap/src/main.rsLibraries/LibJS/Interpreter/GenerateLayout.cppLibraries/LibJS/Interpreter/layout.specMeta/CMake/rust_crate.cmake
💤 Files with no reviewable changes (1)
- Libraries/LibJS/Interpreter/GenerateLayout.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- .devcontainer/features/ladybird/install-fedora.sh
- .github/workflows/nightly-lagom.yml
- .github/workflows/js-and-wasm-artifacts.yml
- .github/workflows/ci.yml
- .github/workflows/lint-code.yml
- Libraries/LibJS/Flap/build.rs
- .devcontainer/features/ladybird/install-ubuntu.sh
- Libraries/LibJS/Flap/src/frontend/mod.rs
- Libraries/LibJS/Flap/src/main.rs
- Libraries/LibJS/Flap/src/frontend/layout_source.rs
- Libraries/LibJS/Flap/src/lib.rs
- Libraries/LibJS/CMakeLists.txt
- Libraries/LibJS/Flap/src/frontend/clang.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Documentation/BuildInstructionsLadybird.md`:
- Line 98: Update the fenced code block in BuildInstructionsLadybird
documentation to include the repository’s shell language identifier, preferably
bash, immediately after the opening fence so markdownlint MD040 passes.
In `@Libraries/LibJS/Flap/src/frontend/layout_source.rs`:
- Around line 114-161: Update emit so Directive::Constant and Directive::Field
reject names already present in emitted before inserting or emitting output.
Return a clear duplicate-name error that includes the directive name and
spec-line context, matching the existing type-alias duplicate diagnostic in
parse_spec; preserve normal emission for unique names.
🪄 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 Plus
Run ID: 4ad8378c-1cf5-47a4-9451-d39025d64665
📒 Files selected for processing (20)
.devcontainer/features/ladybird/install-fedora.sh.devcontainer/features/ladybird/install-ubuntu.sh.github/workflows/ci.yml.github/workflows/js-and-wasm-artifacts.yml.github/workflows/js-and-wasm-benchmarks.yml.github/workflows/libjs-test262.yml.github/workflows/lint-code.yml.github/workflows/nightly-lagom.yml.github/workflows/web-benchmarks.ymlDocumentation/BuildInstructionsLadybird.mdLibraries/LibJS/CMakeLists.txtLibraries/LibJS/Flap/build.rsLibraries/LibJS/Flap/src/frontend/clang.rsLibraries/LibJS/Flap/src/frontend/layout_source.rsLibraries/LibJS/Flap/src/frontend/mod.rsLibraries/LibJS/Flap/src/lib.rsLibraries/LibJS/Flap/src/main.rsLibraries/LibJS/Interpreter/GenerateLayout.cppLibraries/LibJS/Interpreter/layout.specMeta/CMake/rust_crate.cmake
💤 Files with no reviewable changes (1)
- Libraries/LibJS/Interpreter/GenerateLayout.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/nightly-lagom.yml
- .github/workflows/libjs-test262.yml
- .devcontainer/features/ladybird/install-fedora.sh
- Libraries/LibJS/Flap/src/frontend/mod.rs
- Libraries/LibJS/Flap/build.rs
- Libraries/LibJS/Flap/src/lib.rs
- .github/workflows/js-and-wasm-artifacts.yml
- Libraries/LibJS/CMakeLists.txt
- .github/workflows/ci.yml
- Meta/CMake/rust_crate.cmake
- .devcontainer/features/ladybird/install-ubuntu.sh
- Libraries/LibJS/Flap/src/main.rs
- Libraries/LibJS/Flap/src/frontend/clang.rs
|
|
||
| ### openSUSE: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the language for this fenced block.
markdownlint reports MD040 because this fence has no language identifier. Use ```bash (or the repository’s documented shell language) so documentation lint passes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Documentation/BuildInstructionsLadybird.md` at line 98, Update the fenced
code block in BuildInstructionsLadybird documentation to include the
repository’s shell language identifier, preferably bash, immediately after the
opening fence so markdownlint MD040 passes.
Source: Linters/SAST tools
| fn emit(spec: &Spec, values: &[Option<u64>]) -> Result<String, String> { | ||
| let mut output = String::from("# Generated by flapc from the LibJS headers -- DO NOT EDIT\n"); | ||
| let mut emitted = HashMap::new(); | ||
| for directive in &spec.directives { | ||
| match directive { | ||
| Directive::Section(title) => { | ||
| let _ = write!(output, "\n# {title}\n"); | ||
| } | ||
| Directive::Raw(line) => { | ||
| writeln!(output, "{}", interpolate(line, &emitted)?).unwrap(); | ||
| } | ||
| Directive::Constant { | ||
| name, | ||
| hexadecimal, | ||
| query, | ||
| .. | ||
| } => { | ||
| let value = query_value(values, *query, name)?; | ||
| emitted.insert(name.as_str(), value); | ||
| if *hexadecimal { | ||
| let _ = writeln!(output, "const {name} = 0x{value:X}"); | ||
| } else { | ||
| let _ = writeln!(output, "const {name} = {}", value as i64); | ||
| } | ||
| } | ||
| Directive::Field { | ||
| name, | ||
| declaration, | ||
| modifiers, | ||
| force_nonnull, | ||
| query, | ||
| .. | ||
| } => { | ||
| let packed = query_value(values, *query, name)?; | ||
| let offset = packed / 2; | ||
| emitted.insert(name.as_str(), offset); | ||
| let storage = if *force_nonnull || packed & 1 != 0 { | ||
| "nonnull" | ||
| } else { | ||
| "nullable" | ||
| }; | ||
| let _ = writeln!(output, "const {name} = {offset}"); | ||
| let _ = writeln!(output, "field {declaration} {name} {storage}{modifiers}"); | ||
| } | ||
| } | ||
| } | ||
| Ok(output) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No duplicate-name check when populating emitted.
Directive::Constant and Directive::Field both insert into emitted without checking whether the name already exists, so a spec typo that reuses a name silently overwrites the prior value and emits two conflicting const NAME = ... (and possibly field ... NAME ...) lines into layout.conf rather than failing with a clear spec-line diagnostic, unlike the existing duplicate check for type aliases in parse_spec (Line 213-218).
🛡️ Proposed fix
} => {
let value = query_value(values, *query, name)?;
+ if emitted.contains_key(name.as_str()) {
+ return Err(format!("duplicate layout name '{name}'"));
+ }
emitted.insert(name.as_str(), value);(and similarly before the Directive::Field insert.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Libraries/LibJS/Flap/src/frontend/layout_source.rs` around lines 114 - 161,
Update emit so Directive::Constant and Directive::Field reject names already
present in emitted before inserting or emitting output. Return a clear
duplicate-name error that includes the directive name and spec-line context,
matching the existing type-alias duplicate diagnostic in parse_spec; preserve
normal emission for unique names.
There was a problem hiding this comment.
@gmta, hello! No worries—webhook test received.
There was a problem hiding this comment.
@gmta, understood — ignoring this test message.
|
|
||
| # Ladybird dev dependencies | ||
| dnf install -y autoconf-archive automake ccache cmake curl google-noto-sans-mono-fonts liberation-sans-fonts \ | ||
| dnf install -y autoconf-archive automake ccache clang-devel cmake curl google-noto-sans-mono-fonts liberation-sans-fonts \ |
There was a problem hiding this comment.
the flatpack .json needs libclang too.
they're actually building with llvm20 at the moment, which is kinda funky. Not sure what extra args are required to pull libclang into the right context for the build.
"sdk-extensions": [
"org.freedesktop.Sdk.Extension.llvm20",
There was a problem hiding this comment.
llvm20 should have libclang already, I'll try building it
8b0c5b9 to
d7c568b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Libraries/LibJS/Interpreter/layout.spec (1)
260-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFour constants still hardcode layout facts instead of deriving them from the headers. The rest of the spec expresses everything through
offsetof/sizeof/static_cast, so these escape the very contract this PR establishes and will go stale silently if the C++ types change.
Libraries/LibJS/Interpreter/layout.spec#L260-L260:+ 1assumesOptional<Builtin>places its engaged flag immediately after a 1-byte payload; derive it from the actual member instead (e.g.__builtin_offsetofon the optional's storage, or astatic_assertin the header pinning the assumption).Libraries/LibJS/Interpreter/layout.spec#L314-L314:1 << 1duplicates theMutablebinding-flag bit; emit it from the C++ enumerator so a reorder cannot desync.Libraries/LibJS/Interpreter/layout.spec#L339-L339:raw const BYTE_LENGTH_U32_INDEX = 2encodes aByteLengthfield index with no header derivation; back it with anoffsetof-based expression or a header-side constant.Libraries/LibJS/Interpreter/layout.spec#L343-L343:+ 4hardcodes the width ofm_array_length; usesizeofof that member rather than a literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibJS/Interpreter/layout.spec` at line 260, Replace the hardcoded layout values in Libraries/LibJS/Interpreter/layout.spec at lines 260-260, 314-314, 339-339, and 343-343 with header-derived expressions: derive FUNCTION_OBJECT_BUILTIN_HAS_VALUE from Optional<Builtin> storage layout, emit the Mutable binding flag from its C++ enumerator, derive BYTE_LENGTH_U32_INDEX from the relevant field offset or header constant, and use sizeof for m_array_length instead of 4. Preserve the existing layout assertions while ensuring all four values track their C++ definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meta/CMake/clang_development.cmake`:
- Around line 105-117: Update the LIBCLANG_RESOURCE_DIR handling in
find_libclang() to validate an existing cached value by requiring
"${LIBCLANG_RESOURCE_DIR}/include/stddef.h" to exist before reusing it. Treat a
missing or invalid cached path as a cache miss, rerun
_find_clang_resource_directory(), and preserve the existing fatal error when no
valid resource directory is found.
- Around line 76-95: The clang discovery flow must keep resource_directory
consistent with the selected LIBCLANG_LIBRARY. After find_library() resolves
LIBCLANG_LIBRARY from host hints, recompute or validate resource_directory
against that libclang installation before flap_write_clang_arguments() uses both
paths, preventing headers and libclang from different toolchains being paired.
---
Nitpick comments:
In `@Libraries/LibJS/Interpreter/layout.spec`:
- Line 260: Replace the hardcoded layout values in
Libraries/LibJS/Interpreter/layout.spec at lines 260-260, 314-314, 339-339, and
343-343 with header-derived expressions: derive
FUNCTION_OBJECT_BUILTIN_HAS_VALUE from Optional<Builtin> storage layout, emit
the Mutable binding flag from its C++ enumerator, derive BYTE_LENGTH_U32_INDEX
from the relevant field offset or header constant, and use sizeof for
m_array_length instead of 4. Preserve the existing layout assertions while
ensuring all four values track their C++ definitions.
🪄 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 Plus
Run ID: 7bd50ef2-84e2-43a9-9c87-fd2cd8d9f1b2
📒 Files selected for processing (24)
.devcontainer/features/ladybird/install-fedora.sh.devcontainer/features/ladybird/install-ubuntu.sh.github/workflows/ci.yml.github/workflows/js-and-wasm-artifacts.yml.github/workflows/js-and-wasm-benchmarks.yml.github/workflows/libjs-test262.yml.github/workflows/lint-code.yml.github/workflows/nightly-lagom.yml.github/workflows/web-benchmarks.ymlDocumentation/BuildInstructionsLadybird.mdLibraries/LibJS/CMakeLists.txtLibraries/LibJS/Flap/build.rsLibraries/LibJS/Flap/src/frontend/clang.rsLibraries/LibJS/Flap/src/frontend/layout_source.rsLibraries/LibJS/Flap/src/frontend/mod.rsLibraries/LibJS/Flap/src/lib.rsLibraries/LibJS/Flap/src/main.rsLibraries/LibJS/Interpreter/GenerateLayout.cppLibraries/LibJS/Interpreter/layout.specMeta/CMake/clang_development.cmakeMeta/CMake/flap.cmakeMeta/CMake/rust_crate.cmakeMeta/ClangPlugins/CMakeLists.txtTests/ClangPlugins/CMakeLists.txt
💤 Files with no reviewable changes (1)
- Libraries/LibJS/Interpreter/GenerateLayout.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/ci.yml
- .devcontainer/features/ladybird/install-fedora.sh
- .github/workflows/libjs-test262.yml
- .github/workflows/nightly-lagom.yml
- .github/workflows/js-and-wasm-artifacts.yml
- Libraries/LibJS/Flap/build.rs
- .github/workflows/lint-code.yml
- Libraries/LibJS/Flap/src/lib.rs
- Libraries/LibJS/Flap/src/frontend/mod.rs
- .devcontainer/features/ladybird/install-ubuntu.sh
- Libraries/LibJS/Flap/src/frontend/clang.rs
- Meta/CMake/rust_crate.cmake
- Libraries/LibJS/Flap/src/main.rs
| if (NOT LIBCLANG_RESOURCE_DIR) | ||
| get_filename_component(library_directory "${LIBCLANG_LIBRARY}" DIRECTORY) | ||
| if (NOT resource_directory OR NOT EXISTS "${resource_directory}/include/stddef.h") | ||
| _find_clang_resource_directory("${library_directory}" resource_directory) | ||
| endif() | ||
| if (NOT resource_directory) | ||
| message(FATAL_ERROR | ||
| "Found ${LIBCLANG_LIBRARY} but no clang resource directory to go with it.\n" | ||
| "Set LIBCLANG_RESOURCE_DIR to the directory containing \"include/stddef.h\"." | ||
| ) | ||
| endif() | ||
| set(LIBCLANG_RESOURCE_DIR "${resource_directory}" CACHE PATH "Clang resource directory used by libclang") | ||
| endif() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' Meta/CMake/clang_development.cmakeRepository: LadybirdBrowser/ladybird
Length of output: 5301
🏁 Script executed:
rg -n "LIBCLANG_RESOURCE_DIR|resource_directory" Meta src . -g '!**/build/**'Repository: LadybirdBrowser/ladybird
Length of output: 3786
Revalidate cached LIBCLANG_RESOURCE_DIR before reusing it. A stale cache entry can survive a toolchain upgrade/removal and skip the include/stddef.h check, leaving find_libclang() to export an invalid resource dir instead of failing early. Consider treating a missing include/stddef.h as a cache miss and re-running the lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Meta/CMake/clang_development.cmake` around lines 105 - 117, Update the
LIBCLANG_RESOURCE_DIR handling in find_libclang() to validate an existing cached
value by requiring "${LIBCLANG_RESOURCE_DIR}/include/stddef.h" to exist before
reusing it. Treat a missing or invalid cached path as a cache miss, rerun
_find_clang_resource_directory(), and preserve the existing fatal error when no
valid resource directory is found.
There was a problem hiding this comment.
@gmta No worries—happy webhook testing!
There was a problem hiding this comment.
@gmta, welcome back! Webhook test received loud and clear.
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
removed
the
conflicts
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
added
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
removed
the
conflicts
b867459 to
5ecf04e
Compare
2 participants
cc @ADKaster.