feat: support more options with js wrapper#60
Merged
fogsong233 merged 8 commits intoclice-io:mainfrom Mar 18, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a generated compiler option subsystem: C++ OptTable implementations and C API bindings, TypeScript types and utilities, generated toolchain enums, parsing/stringifying helpers, tests, and build/config updates to expose option metadata and streaming parse functionality to JS. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript API
participant CAPI as C API Layer
participant OptTable as OptTable Provider
participant Parser as Option Parser
JS->>CAPI: option_parse(tableName, args[], callback)
CAPI->>OptTable: resolve_table(tableName)
OptTable-->>CAPI: OptTable reference
CAPI->>Parser: parse(args)
loop per parsed span
Parser->>CAPI: ParsedArgument or error string
CAPI->>CAPI: make_option_item / convert values
CAPI->>JS: invoke callback(OptionItem | error)
end
JS->>CAPI: option_get_info(tableName, id)
CAPI->>OptTable: lookup by id
OptTable-->>CAPI: OptionInfo
CAPI-->>JS: return OptionInfo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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 |
![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: 13
🧹 Nitpick comments (6)
api/src/option/lld-coff.ts (1)
1-2: Strip workstation-specific paths from the generated banner.Embedding
/home/kacent/...in a checked-in artifact leaks local machine details and makes regenerations vary by workstation. Please normalize this in the generator to a stable logical source name instead.♻️ Suggested output shape
-// Source: /home/kacent/project/catter/.pixi/envs/dev/include/llvm-options-td/lld-COFF-Options.inc +// Source: llvm-options-td/lld-COFF-Options.inc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/option/lld-coff.ts` around lines 1 - 2, The generated banner in the top of api/src/option/lld-coff.ts contains a workstation-specific absolute path; update the generator (scripts/js-gen/generate_llvm_option.mjs) so it normalizes source paths before writing the header (e.g., strip directories and/or replace leading absolute prefixes with a stable logical name like "llvm-options-td/lld-COFF-Options.inc" or use path.basename), by matching the absolute path with a regex and substituting the normalized value when composing the banner comment; ensure the change targets the banner-generation code path so regenerated files no longer embed local /home/... paths.scripts/package.json (1)
5-10: Updategenerate:optionto refresh both LLVM and NVCC artifacts.The
scripts/js-gen/generate_nvcc_option.mjsgenerator exists but is not included in thegenerate:optionentrypoint, which only runs the LLVM generator. This creates a maintenance gap where developers using the generic regeneration command will leave NVCC artifacts stale.Update the
generate:optionscript to run both generators:"generate:option": "pnpm run generate:llvm-option && pnpm run generate:nvcc-option"Also add a corresponding script for the NVCC generator:
"generate:nvcc-option": "node ./js-gen/generate_nvcc_option.mjs"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package.json` around lines 5 - 10, Update the package.json scripts so the generic generator runs both LLVM and NVCC generators: change the "generate:option" script (currently invoking "generate:llvm-option") to run "generate:llvm-option" AND "generate:nvcc-option" in sequence, and add a new "generate:nvcc-option" script that invokes the NVCC generator entrypoint (the existing js-gen/generate_nvcc_option.mjs). Ensure you reference the existing "generate:llvm-option" and add the new "generate:nvcc-option" script name so the single command refreshes both artifact sets.src/common/opt/external/lld_coff.h (1)
3-29: Forward-declareOptTablein these headers and add explicit includes to their implementations.These 9 headers in
src/common/opt/external/all include<eventide/option/opt_table.h>but only need it for the function declarationconst eventide::option::OptTable& table();. Switching to a forward declaration would reduce coupling. However, the corresponding.ccfiles currently lack an explicit include—they getOptTabletransitively through their.hincludes. The refactoring requires adding#include <eventide/option/opt_table.h>to all 9.ccfiles.♻️ Suggested header change (apply to all 9 files)
-#include <eventide/option/opt_table.h> +namespace eventide { +namespace option { +class OptTable; +} +}Affected headers:
clang.h,lld_coff.h,lld_elf.h,lld_macho.h,lld_mingw.h,lld_wasm.h,llvm_dlltool.h,llvm_lib.h,nvcc.h.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/opt/external/lld_coff.h` around lines 3 - 29, Replace the heavy include in each header (e.g., src/common/opt/external/lld_coff.h) by forward-declaring class OptTable in namespace eventide::option and keeping only the declaration const eventide::option::OptTable& table();; then in each corresponding implementation (.cc) file for the nine headers (clang, lld_coff, lld_elf, lld_macho, lld_mingw, lld_wasm, llvm_dlltool, llvm_lib, nvcc) add an explicit `#include` <eventide/option/opt_table.h> so the definition of eventide::option::OptTable is available where table() is defined. Ensure the header no longer includes opt_table.h and the .cc files include it.src/common/opt/external/clang.cc (1)
65-75: Make the prefix-layout guard exhaustive.Line 73 still collapses any new
PREFIXES_OFFSETtopfx_none. The currentstatic_asserts pin today's entries, but they do not assert that LLVM cannot append another prefix block later, so this can still silently misparse instead of failing at compile time.🔒 Lightweight guard
static_assert(OptionPrefixesTable[8] == 3 && OptionPrefixesTable[9] == 3 && OptionPrefixesTable[10] == 6 && OptionPrefixesTable[11] == 1); static_assert(OptionPrefixesTable[12] == 2 && OptionPrefixesTable[13] == 6 && OptionPrefixesTable[14] == 1); +static_assert(sizeof(OptionPrefixesTable) / sizeof(OptionPrefixesTable[0]) == 15, + "Update clang::prefixes when LLVM adds a new prefix layout");Also applies to: 86-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/opt/external/clang.cc` around lines 65 - 75, The switch in prefixes(unsigned) currently falls back to pfx_none for unknown offsets; add a compile-time guard that makes unknown PREFIXES_OFFSET fail the build: add a static_assert that PREFIXES_OFFSET is one of the allowed values (0,1,3,6,8,12) and remove/replace the permissive default with a compile-time unreachable (e.g. a static_assert(false, "...") or no default so the compiler errors for unhandled cases) so any future LLVM append will break the build; apply the same change to the analogous switch block at lines 86-97. Ensure you reference the PREFIXES_OFFSET constant and the eo::pfx_* symbols in these checks so the guard is exhaustive.tests/unit/common/opt/clang.cc (1)
49-118: Add one slash-prefixed clang case.These tests only exercise
-and--spellings, but the new clang table bridge also maps/-based prefixes. A small/Ifooor/Fofoo.objcase would keep that branch from regressing unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/common/opt/clang.cc` around lines 49 - 118, Add a test exercising slash-prefixed clang options: call parse_command with an argv containing "/Iinclude" and "/Fooutput.obj" (or a single-case "/Iinclude" if you prefer minimal), then assert parsed.errors.empty(), check parsed.args contains the corresponding option entries and that their option_id.id() match opt::clang::ID_I and opt::clang::ID_o (or the bridge-equivalent IDs) and that their values[0] are "include" and "output.obj" (or "foo.obj"); add this under the existing TEST_SUITE(clang_option_table_tests) (e.g., as a new TEST_CASE or appended to parse_clang_compile_command) next to other parse_command checks so the slash-prefix branch is exercised.scripts/js-gen/generate_llvm_option.mjs (1)
117-119: Subset generation should not rewrite a fullindex.ts.
printHelp()advertisesnode ... clang, but Line 118 always emits exports for every table plusnvcc. On a workspace missing one of those omitted outputs, a single-table run leavesindex.tsimporting files that were never generated.♻️ Suggested change
- const indexFile = path.join(outputDir, "index.ts"); - await writeFile(indexFile, renderIndexModule([...TABLES, ...EXTRA_EXPORT_TABLES])); - process.stdout.write(`generated ${path.relative(process.cwd(), indexFile)}\n`); + const generatingAllTables = + new Set(selectedTables.map((table) => table.key)).size === TABLES.length; + if (generatingAllTables) { + const indexFile = path.join(outputDir, "index.ts"); + await writeFile( + indexFile, + renderIndexModule([...TABLES, ...EXTRA_EXPORT_TABLES]), + ); + process.stdout.write(`generated ${path.relative(process.cwd(), indexFile)}\n`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/js-gen/generate_llvm_option.mjs` around lines 117 - 119, The current code always rewrites index.ts with exports for every table (using [...TABLES, ...EXTRA_EXPORT_TABLES]), which breaks subset runs; change the logic so you compute the actual list of tables generated in this run (e.g., the array of table names you created earlier or by filtering TABLES/EXTRA_EXPORT_TABLES to those files you wrote) and pass that filtered list to renderIndexModule instead of the full spread; then call writeFile(indexFile, renderIndexModule(generatedTables)) (or skip writing the index if no tables were produced) so index.ts only references files that were actually generated; keep using indexFile, renderIndexModule, writeFile and process.stdout.write as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/catter-c/capi.d.ts`:
- Around line 266-280: The declaration export enum OptionKindClass currently
claims a runtime export that the native binding does not provide, causing
runtime import failures; either (A) export the enum from the native binding
under the exact symbol OptionKindClass so consumers can import the runtime
value, or (B) change the .d.ts to a type-only declaration (replace the runtime
enum with a type alias like export type OptionKindClass = /* numeric-literal
union matching enum values */) and ensure code that relies on a runtime value
keeps its local enum/const, so no module attempts to import OptionKindClass at
runtime.
In `@api/src/index.ts`:
- Line 24: The re-export "export type { OptionItem, OptionTable } from
\"catter-c\";" produces an unresolved bare import in the published .d.ts; fix by
either adding "catter-c" to bundledPackages in api-extractor.json so its types
are inlined into the rollup output, or replace the re-export with a relative
vendored type shim (e.g., change the import target to a shipped file like
"./capi" that re-exports OptionItem and OptionTable) and update the export line
accordingly; locate the export in api/src/index.ts and make the corresponding
change and/or update api-extractor.json.
In `@api/src/option/lld-macho.ts`:
- Around line 1-2: The generated header comment in api/src/option/lld-macho.ts
currently contains an absolute local path (from
scripts/js-gen/generate_llvm_option.mjs output); update that comment to remove
the machine-specific path and instead include a repo-relative path or just the
basename (e.g., "Source: llvm-options-td/lld-MachO-Options.inc" or similar), and
update the generator (scripts/js-gen/generate_llvm_option.mjs) so future
regenerations emit the sanitized source marker rather than the full absolute
path.
In `@api/src/option/lld-wasm.ts`:
- Around line 1-2: The committed generated header embeds a developer-local
absolute path in the "// Source:" comment in the lld-wasm.ts output; update the
generator (scripts/js-gen/generate_llvm_option.mjs) to sanitize that metadata so
it emits either a repo-relative path, a stable placeholder, or strips the local
prefix before writing the "// Source:" line in the generated file (or add a
post-generation step that rewrites the "// Source:" comment); ensure the change
targets the generator that produces the "// Source:" comment so future
regenerations don't reintroduce the absolute local path.
In `@api/src/option/llvm-lib.ts`:
- Around line 1-2: The checked-in file api/src/option/llvm-lib.ts contains a
machine-local absolute path on line 2 coming from the generator
scripts/js-gen/generate_llvm_option.mjs; modify that generator to sanitize or
replace absolute source paths (e.g., /home/<user>/.../llvm-lib-Options.inc) with
a repo-relative label or a stable tool/version string, update the template that
emits the header comment so it uses the sanitized value, then re-run
scripts/js-gen/generate_llvm_option.mjs to regenerate api/src/option/llvm-lib.ts
with the repo-relative or versioned source reference instead of the
developer-local absolute path.
In `@scripts/js-gen/generate_llvm_option.mjs`:
- Around line 8-19: The FLAG_BITS map is missing the RenderSeparate entry (bit
3) causing a mismatch with code that expects RENDER_SEPARATE = 1 << 3; add
"RenderSeparate: 1 << 3" to the FLAG_BITS object next to RenderJoined so the bit
ordering matches the generator's use in renderTokens; this will ensure
OptionInfo.flags and enums like ClangFlag/LldElfFlag correctly include and
interpret the RenderSeparate flag.
In `@scripts/js-gen/generate_nvcc_option.mjs`:
- Around line 88-95: The current fallback uses execFileSync("bash", ["-lc",
"command -v nvcc"], ...) which fails on Windows; update the logic in
generate_nvcc_option.mjs to check process.platform === 'win32' and call
execFileSync with where.exe (e.g., execFileSync("where.exe", ["nvcc"], {
encoding: "utf8" })) on Windows, otherwise use execFileSync("command", ["-v",
"nvcc"], { encoding: "utf8" }) or similar for Unix, trim and return the result
if non-empty, and preserve the existing try/catch behavior around execFileSync
to silently continue on failures; reference the execFileSync usage and nvcc
lookup code to locate the change.
- Around line 243-305: The code calls normalizeMetaVar(option.valueSyntax) to
compute metaVar but then always emits four variant entries (Separate, Joined,
JoinedOrSeparate, Joined) regardless of the parsed syntax; update the emission
logic in the block that builds optionEntries (the optionEntries.push calls, and
the idAllocator/sanitizeIdentifier usage for alias IDs) to consult
option.valueSyntax/metaVar and only emit the forms allowed by that syntax (e.g.,
if valueSyntax indicates only joined form, do not create the Separate or
JoinedOrSeparate entries), or alternatively add tests proving that all four
forms are intentionally accepted by supported nvcc versions; specifically change
the branches that currently unconditionally add the Joined entry and the
JoinedOrSeparate alias entry to be conditional based on the normalized
metaVar/valueSyntax shape.
In `@src/catter/core/capi/option.cc`:
- Around line 134-140: The error branch currently forwards only a raw string
from table.parse_args within the lambda passed to table.parse_args, which loses
token boundary info; update the error path in that lambda (the branch that calls
emit_callback_value(callback, catter::qjs::Value::from(ctx, parsed.error()))) to
construct and emit a structured payload that includes both the error message and
the failing index/span (e.g., fields like "message" and "span" or "index") so
callers (e.g., option.replace) can recover the exact token boundaries; use the
same helper/value construction path used for successful results
(emit_callback_value + catter::qjs::Value::from) and populate the object before
emitting instead of sending the raw string.
In `@src/catter/core/js.cc`:
- Line 3: Replace the incorrect C++23 header by removing the `#include <print>`
and add `#include <format>` so `std::format` usages (lines referenced around the
`std::format` calls at locations where formatting is done) are satisfied
explicitly; update the includes in src/catter/core/js.cc (look for the
top-of-file include block and the functions that call std::format around lines
where formatting occurs) to remove the unused `<print>` and include `<format>`
instead.
In `@tests/unit/catter/core/js.cc`:
- Around line 71-74: The catch block handling catter::qjs::Exception& ex in
tests/unit/catter/core/js.cc should rethrow the original exception using throw;
instead of throw ex; to preserve the original exception context and stack
trace—locate the catch(catter::qjs::Exception& ex) block (which currently calls
catter::output::redLn("{}", ex.what())) and replace the throw ex; with a plain
throw; so the exception is propagated without slicing or losing diagnostic
information.
In `@tests/unit/common/opt/clang.cc`:
- Around line 1-4: The file uses std::span and std::string_view but only relies
on transitive includes; add explicit includes for <span> and <string_view>
alongside the existing headers at the top of the file so usages of std::span and
std::string_view (the symbols referenced in the code) are guaranteed to compile
without fragile transitive dependencies.
In `@tests/unit/common/opt/external.cc`:
- Around line 30-31: In parse_command (ParseResult parse_command(const
eo::OptTable& table, std::span<const std::string> argv)), guard against an empty
argv before using argv.begin() + 1: check argv.empty() (or argv.size() <= 1) and
construct args accordingly (e.g., use an empty vector if argv is empty, or copy
from argv.begin() + 1 only when size > 0) so you avoid undefined behavior when
the span is empty; keep the rest of the parsing logic unchanged.
---
Nitpick comments:
In `@api/src/option/lld-coff.ts`:
- Around line 1-2: The generated banner in the top of api/src/option/lld-coff.ts
contains a workstation-specific absolute path; update the generator
(scripts/js-gen/generate_llvm_option.mjs) so it normalizes source paths before
writing the header (e.g., strip directories and/or replace leading absolute
prefixes with a stable logical name like "llvm-options-td/lld-COFF-Options.inc"
or use path.basename), by matching the absolute path with a regex and
substituting the normalized value when composing the banner comment; ensure the
change targets the banner-generation code path so regenerated files no longer
embed local /home/... paths.
In `@scripts/js-gen/generate_llvm_option.mjs`:
- Around line 117-119: The current code always rewrites index.ts with exports
for every table (using [...TABLES, ...EXTRA_EXPORT_TABLES]), which breaks subset
runs; change the logic so you compute the actual list of tables generated in
this run (e.g., the array of table names you created earlier or by filtering
TABLES/EXTRA_EXPORT_TABLES to those files you wrote) and pass that filtered list
to renderIndexModule instead of the full spread; then call writeFile(indexFile,
renderIndexModule(generatedTables)) (or skip writing the index if no tables were
produced) so index.ts only references files that were actually generated; keep
using indexFile, renderIndexModule, writeFile and process.stdout.write as
before.
In `@scripts/package.json`:
- Around line 5-10: Update the package.json scripts so the generic generator
runs both LLVM and NVCC generators: change the "generate:option" script
(currently invoking "generate:llvm-option") to run "generate:llvm-option" AND
"generate:nvcc-option" in sequence, and add a new "generate:nvcc-option" script
that invokes the NVCC generator entrypoint (the existing
js-gen/generate_nvcc_option.mjs). Ensure you reference the existing
"generate:llvm-option" and add the new "generate:nvcc-option" script name so the
single command refreshes both artifact sets.
In `@src/common/opt/external/clang.cc`:
- Around line 65-75: The switch in prefixes(unsigned) currently falls back to
pfx_none for unknown offsets; add a compile-time guard that makes unknown
PREFIXES_OFFSET fail the build: add a static_assert that PREFIXES_OFFSET is one
of the allowed values (0,1,3,6,8,12) and remove/replace the permissive default
with a compile-time unreachable (e.g. a static_assert(false, "...") or no
default so the compiler errors for unhandled cases) so any future LLVM append
will break the build; apply the same change to the analogous switch block at
lines 86-97. Ensure you reference the PREFIXES_OFFSET constant and the eo::pfx_*
symbols in these checks so the guard is exhaustive.
In `@src/common/opt/external/lld_coff.h`:
- Around line 3-29: Replace the heavy include in each header (e.g.,
src/common/opt/external/lld_coff.h) by forward-declaring class OptTable in
namespace eventide::option and keeping only the declaration const
eventide::option::OptTable& table();; then in each corresponding implementation
(.cc) file for the nine headers (clang, lld_coff, lld_elf, lld_macho, lld_mingw,
lld_wasm, llvm_dlltool, llvm_lib, nvcc) add an explicit `#include`
<eventide/option/opt_table.h> so the definition of eventide::option::OptTable is
available where table() is defined. Ensure the header no longer includes
opt_table.h and the .cc files include it.
In `@tests/unit/common/opt/clang.cc`:
- Around line 49-118: Add a test exercising slash-prefixed clang options: call
parse_command with an argv containing "/Iinclude" and "/Fooutput.obj" (or a
single-case "/Iinclude" if you prefer minimal), then assert
parsed.errors.empty(), check parsed.args contains the corresponding option
entries and that their option_id.id() match opt::clang::ID_I and
opt::clang::ID_o (or the bridge-equivalent IDs) and that their values[0] are
"include" and "output.obj" (or "foo.obj"); add this under the existing
TEST_SUITE(clang_option_table_tests) (e.g., as a new TEST_CASE or appended to
parse_clang_compile_command) next to other parse_command checks so the
slash-prefix branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b32468de-6841-430f-8ade-0b96afdcaf91
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (58)
api/catter-c/capi.d.tsapi/src/debug.tsapi/src/fs.tsapi/src/index.tsapi/src/io.tsapi/src/option/clang.tsapi/src/option/index.tsapi/src/option/lld-coff.tsapi/src/option/lld-elf.tsapi/src/option/lld-macho.tsapi/src/option/lld-mingw.tsapi/src/option/lld-wasm.tsapi/src/option/llvm-dlltool.tsapi/src/option/llvm-lib.tsapi/src/option/nvcc.tsapi/src/os.tsapi/src/service.tsapi/test/option.tsapi/tsconfig.test.jsonpixi.tomlscripts/js-gen/generate_llvm_option.mjsscripts/js-gen/generate_nvcc_option.mjsscripts/package.jsonsrc/catter-proxy/ipc.ccsrc/catter-proxy/main.ccsrc/catter/core/capi/option.ccsrc/catter/core/ipc.hsrc/catter/core/js.ccsrc/catter/core/session.hsrc/catter/main.ccsrc/common/opt/external/clang.ccsrc/common/opt/external/clang.hsrc/common/opt/external/lld_coff.ccsrc/common/opt/external/lld_coff.hsrc/common/opt/external/lld_elf.ccsrc/common/opt/external/lld_elf.hsrc/common/opt/external/lld_macho.ccsrc/common/opt/external/lld_macho.hsrc/common/opt/external/lld_mingw.ccsrc/common/opt/external/lld_mingw.hsrc/common/opt/external/lld_wasm.ccsrc/common/opt/external/lld_wasm.hsrc/common/opt/external/llvm_dlltool.ccsrc/common/opt/external/llvm_dlltool.hsrc/common/opt/external/llvm_lib.ccsrc/common/opt/external/llvm_lib.hsrc/common/opt/external/nvcc.ccsrc/common/opt/external/nvcc.hsrc/common/opt/external/tablegen.hsrc/common/util/eventide.hsrc/common/util/packet_io.hsrc/common/util/serde.htests/integration/test/catter-proxy.cctests/unit/base/main.cctests/unit/catter/core/js.cctests/unit/common/opt/clang.cctests/unit/common/opt/external.ccxmake.lua
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/test/option.ts (1)
3-31: Remove duplicateOptionKindClassconstant; import from the public API instead.The test file duplicates the enum definition from
api/src/option/types.ts, which can silently drift. SinceOptionKindClassis exported from the "catter" package'soptionnamespace, replace the local constant with an import:import { debug, option } from "catter"; -const OptionKindClass: { - GroupClass: number; - InputClass: number; - UnknownClass: number; - FlagClass: number; - JoinedClass: number; - ValuesClass: number; - SeparateClass: number; - RemainingArgsClass: number; - RemainingArgsJoinedClass: number; - CommaJoinedClass: number; - MultiArgClass: number; - JoinedOrSeparateClass: number; - JoinedAndSeparateClass: number; -} = { - GroupClass: 0, - InputClass: 1, - UnknownClass: 2, - FlagClass: 3, - JoinedClass: 4, - ValuesClass: 5, - SeparateClass: 6, - RemainingArgsClass: 7, - RemainingArgsJoinedClass: 8, - CommaJoinedClass: 9, - MultiArgClass: 10, - JoinedOrSeparateClass: 11, - JoinedAndSeparateClass: 12, -}; +const { OptionKindClass } = option;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/test/option.ts` around lines 3 - 31, Remove the duplicated OptionKindClass constant in api/test/option.ts and import the canonical definition from the public API instead; delete the local OptionKindClass object and add an import that references the package's option namespace (e.g., import the exported option namespace from "catter" and use option.OptionKindClass or destructure OptionKindClass from that namespace) so the test uses the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/test/option.ts`:
- Around line 3-31: Remove the duplicated OptionKindClass constant in
api/test/option.ts and import the canonical definition from the public API
instead; delete the local OptionKindClass object and add an import that
references the package's option namespace (e.g., import the exported option
namespace from "catter" and use option.OptionKindClass or destructure
OptionKindClass from that namespace) so the test uses the single source of
truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c64469d-8dc8-4ba4-a016-5dfc730c83f9
📒 Files selected for processing (4)
api/src/index.tsapi/src/option/index.tsapi/src/option/types.tsapi/test/option.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/index.ts
- api/src/option/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/option/nvcc.ts (1)
405-407: Consider whetherNvccFlagis intentionally minimal.The enum contains only
None = 0. If the NVCC option table genuinely defines no metadata flags, this is correct. However, if this is a placeholder or the generation missed flag definitions, consumers checkinginfo.flagswould always see0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/option/nvcc.ts` around lines 405 - 407, The NvccFlag enum currently only defines None = 0 which will make any consumer reading info.flags always see 0; verify whether this is intentionally minimal or a generation bug by checking the NVCC option table/schema used to build api/src/option/nvcc.ts, and if flags exist add the missing named bitflags to the NvccFlag enum (preserving bit values like 1<<n), update any consumers referencing info.flags to handle the new flags, and if it is intentional add a clear comment/TODO in NvccFlag explaining it is deliberately empty or fix the generator that produces NvccFlag so future builds include all defined flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/option/nvcc.ts`:
- Around line 405-407: The NvccFlag enum currently only defines None = 0 which
will make any consumer reading info.flags always see 0; verify whether this is
intentionally minimal or a generation bug by checking the NVCC option
table/schema used to build api/src/option/nvcc.ts, and if flags exist add the
missing named bitflags to the NvccFlag enum (preserving bit values like 1<<n),
update any consumers referencing info.flags to handle the new flags, and if it
is intentional add a clear comment/TODO in NvccFlag explaining it is
deliberately empty or fix the generator that produces NvccFlag so future builds
include all defined flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5ef4951-681c-461f-9828-d62e4ad20d9b
📒 Files selected for processing (8)
api/src/option/lld-coff.tsapi/src/option/lld-elf.tsapi/src/option/lld-macho.tsapi/src/option/lld-mingw.tsapi/src/option/lld-wasm.tsapi/src/option/llvm-dlltool.tsapi/src/option/llvm-lib.tsapi/src/option/nvcc.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- api/src/option/lld-mingw.ts
- api/src/option/lld-macho.ts
- api/src/option/llvm-dlltool.ts
- api/src/option/lld-elf.ts
- api/src/option/lld-wasm.ts
This was referenced Mar 18, 2026
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