Repository navigation
feat: add SearchConfig, ToolchainProvider, PathPool and related tests - #370
Merged
Merged
Conversation
3 tasks done
|
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 cached ToolchainProvider, header-search extraction and PathPool intern, extends CompilationDatabase with toolchain/path resolution and caching, updates CMake, and adds unit tests and test utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CompilationDatabase
participant ToolchainProvider
participant ToolchainSystem as toolchain::query_toolchain
participant ArgParser as ArgumentParser
Client->>CompilationDatabase: lookup(file, options.query_toolchain=true)
CompilationDatabase->>ToolchainProvider: query_cached(file, directory, user_args)
alt Cache Hit
ToolchainProvider-->>CompilationDatabase: cc1_args (cached)
else Cache Miss
ToolchainProvider->>ArgParser: parse(user_args, filter_toolchain_options)
ArgParser-->>ToolchainProvider: toolchain_relevant_args
ToolchainProvider->>ToolchainSystem: query_toolchain(filtered_args, callback)
ToolchainSystem-->>ToolchainProvider: cc1_args (via callback)
ToolchainProvider->>ToolchainProvider: insert into cache
ToolchainProvider-->>CompilationDatabase: cc1_args
end
CompilationDatabase->>ArgParser: parse(original_user_args, extract_include_flags)
ArgParser-->>CompilationDatabase: include_flags (-I, -isystem, -iquote, ...)
CompilationDatabase->>CompilationDatabase: merge cc1_args + include_flags, fix -resource-dir, update -main-file-name
CompilationDatabase-->>Client: return resolved compilation/search config
sequenceDiagram
participant Caller
participant extract_search_config
participant ArgParser
participant PathNormalizer
Caller->>extract_search_config: arguments[], directory
extract_search_config->>ArgParser: parse(arguments)
loop per include option
ArgParser-->>extract_search_config: option (-iquote, -I, -isystem, -idirafter, -iprefix, -iwithprefix*)
extract_search_config->>extract_search_config: append to appropriate group (quoted/angled/system/after)
end
extract_search_config->>PathNormalizer: resolve_relative_paths(groups, base=directory)
PathNormalizer-->>extract_search_config: absolute_paths
extract_search_config->>extract_search_config: deduplicate [angled_start_idx..end)
extract_search_config-->>Caller: SearchConfig{dirs, angled_start_idx, system_start_idx, after_start_idx}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/clice.cc (1)
77-78: Consider adding error handling for empty executable path.According to the summary, the previous implementation had error logging for resource directory initialization failures. The current code proceeds silently if
getMainExecutablereturns an empty string, which could pass an invalid path toMasterServerwithout any user notification.🛡️ Suggested improvement
static int anchor; std::string self_path = llvm::sys::fs::getMainExecutable("", &anchor); + if(self_path.empty()) { + LOG_ERROR("failed to determine executable path"); + return 1; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clice.cc` around lines 77 - 78, The code does not handle the case where llvm::sys::fs::getMainExecutable returns an empty self_path; update the logic around the static int anchor and std::string self_path to check if self_path.empty() after calling getMainExecutable and, if empty, log an error (e.g., via existing logging mechanism) and abort initialization or return a non-zero exit code instead of passing an invalid path into MasterServer; ensure the error message clearly mentions getMainExecutable() failure and that it prevents resource directory initialization so users see the failure.src/support/path_pool.h (1)
14-17: EncapsulatePathPoolstorage to protect invariants.
allocator,paths, andcacheare publicly mutable, so callers can desynccache[id]↔paths[id]. This risk mirrors the direct mutation pattern visible insrc/index/project_index.cpp(snippet ranges 1-50 and 1-100). Prefer private members with API-only mutation.Refactor sketch
-struct PathPool { - llvm::BumpPtrAllocator allocator; - llvm::SmallVector<llvm::StringRef> paths; - llvm::StringMap<std::uint32_t> cache; +class PathPool { +public: + std::uint32_t intern(llvm::StringRef path); + llvm::StringRef resolve(std::uint32_t id) const; + std::size_t size() const { return paths.size(); } + +private: + llvm::BumpPtrAllocator allocator; + llvm::SmallVector<llvm::StringRef> paths; + llvm::StringMap<std::uint32_t> cache; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/path_pool.h` around lines 14 - 17, PathPool currently exposes allocator, paths, and cache as public members which allows external code to mutate them and break the invariant that cache[id] corresponds to paths[id]; make these three members private in struct PathPool and provide a minimal API (e.g., addPath/lookupOrAdd/getPathById/clear/removeLast) that performs all mutations and keeps cache and paths in sync (use allocator for storage when creating StringRef entries, update llvm::SmallVector paths and llvm::StringMap cache together inside these methods). Update all callers (e.g., uses in project_index.cpp) to use the new API instead of touching allocator/paths/cache directly so invariants are preserved.CMakeLists.txt (1)
17-17: Minor: Comment formatting appears accidentally corrupted.Several comments have unusual formatting that looks unintentional:
- Line 17:
#Make sure→ should be# Make sure- Line 63:
#https: // conda-forge→ should be# https://conda-forge- Line 88:
#, / OPT : NOICF→ appears malformed, possibly meant to be# /OPT:NOICF- Line 132:
#Temporary→ should be# TemporaryThese may have been introduced by an auto-formatter or merge artifact.
Also applies to: 63-63, 88-88, 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 17, Update the malformed comments in CMakeLists.txt: change "#Make sure all third libraries are affected by ABI related options" to include a space after the hash ("# Make sure..."), fix the URL comment at the location with "https: // conda-forge" to "# https://conda-forge", correct the malformed linker flag comment from "#, / OPT : NOICF" to a proper form like "# /OPT:NOICF", and add a space for "# Temporary" instead of "#Temporary"; locate these comments by their current text snippets to apply the exact replacements.tests/unit/test/temp_dir.h (2)
59-67: Silent failure intouch()may cause confusing test failures.When file creation fails (e.g.,
raw_fd_ostreamconstructor setsec), the function silently returns without writing content. This could lead to hard-to-debug test failures where files exist but are empty or missing entirely.♻️ Optional: Add assertion for test helper reliability
void touch(llvm::StringRef relative, llvm::StringRef content = "") { auto p = path(relative); llvm::sys::fs::create_directories(llvm::sys::path::parent_path(p)); std::error_code ec; llvm::raw_fd_ostream out(p, ec); - if(!ec) { - out << content; - } + assert(!ec && "Failed to create file in TempDir::touch()"); + out << content; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/temp_dir.h` around lines 59 - 67, touch() silently ignores file-creation errors from llvm::raw_fd_ostream which leads to confusing test failures; update touch (and its use of path() and llvm::sys::fs::create_directories) to check the std::error_code ec after constructing llvm::raw_fd_ostream and fail fast when ec indicates an error—either assert/llvm_unreachable or throw a runtime_error (with the ec.message() included) so tests immediately surface file creation problems instead of silently returning.
26-28: Consider checkingcreateUniqueDirectoryreturn value.The constructor ignores the error code from
llvm::sys::fs::createUniqueDirectory. If directory creation fails (e.g., insufficient permissions, disk full),rootmay be left in an invalid state, causing subsequent operations to fail silently or operate on unexpected paths.♻️ Suggested improvement
TempDir(llvm::StringRef prefix = "clice-test") { - llvm::sys::fs::createUniqueDirectory(prefix, root); + auto ec = llvm::sys::fs::createUniqueDirectory(prefix, root); + assert(!ec && "Failed to create temporary directory"); + (void)ec; // Suppress unused warning in release builds }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test/temp_dir.h` around lines 26 - 28, The TempDir constructor currently calls llvm::sys::fs::createUniqueDirectory(prefix, root) and ignores its return value; check the returned llvm::ErrorOr/std::error_code and handle failures (e.g., throw a runtime_error, assert, or propagate the error) instead of proceeding with an invalid root. Update the TempDir constructor to inspect the result of createUniqueDirectory, log or propagate the error on failure, and ensure root is only used/set when creation succeeds (refer to the TempDir constructor, createUniqueDirectory call, and the root member when implementing the check).src/command/command.cpp (1)
161-163: Consider documentingoptions_bitsextensibility.Currently
options_bitsonly encodesquery_toolchain. If otherCommandOptionsfields affectSearchConfigresults in the future, this function will need to be updated. A brief comment could help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 161 - 163, The helper options_bits currently encodes only the query_toolchain flag and lacks documentation about future extensibility; add a short comment above the static std::uint8_t options_bits(const CommandOptions& options) function to state that it returns a bitmask used by SearchConfig, list that bit 0 represents query_toolchain, and instruct maintainers to update this bitmask and corresponding SearchConfig logic whenever new CommandOptions fields that influence search results are added (or consider switching to an enum-based bitset if many flags are expected).src/command/toolchain_provider.h (1)
14-20: Clarify lifetime requirements forToolchainQueryfields.
fileanddirectoryarellvm::StringRef(non-owning). The caller must ensure these remain valid until the query is executed. Consider documenting this or usingstd::stringfor safer ownership if queries might be held across async boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/toolchain_provider.h` around lines 14 - 20, ToolchainQuery's file and directory members are llvm::StringRef (non-owning) so their backing data must outlive the query; update the declaration or docs to make this explicit: either change ToolchainQuery::file and ToolchainQuery::directory to std::string to own the data (and adjust constructors/uses like wherever ToolchainQuery is constructed) or add a clear comment above struct ToolchainQuery documenting the lifetime requirement that callers must ensure the referenced strings remain valid until execution (and audit call sites that enqueue queries across async boundaries to copy into std::string if needed).tests/unit/command/toolchain_provider_tests.cpp (1)
28-45: Consider verifying which value is retained on duplicate key injection.The test verifies that
inject_resultshandles duplicate keys without crashing and that the cache has entries afterward, but it doesn't verify that the first result is retained (as the comment on line 42 suggests). If this behavior is important, consider adding an assertion usingquery_cachedto verify the retained value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/command/toolchain_provider_tests.cpp` around lines 28 - 45, Add an assertion that verifies which value is retained when duplicate keys are injected: after calling ToolchainProvider::inject_results with two ToolchainResult entries having the same key ("key1"), call ToolchainProvider::query_cached (or the appropriate lookup method) for "key1" and assert that the returned ToolchainResult matches the first injected entry (the one with "-triple", "x86_64"); reference ToolchainProvider, inject_results, query_cached, and ToolchainResult to locate where to add this check in the TEST_CASE.src/command/search_config.cpp (1)
59-67: Consider documenting behavior when-iwithprefix/-iwithprefixbeforeare used without a preceding-iprefix.If
-iwithprefix diris used without a prior-iprefix, theprefixstring is empty, so the result is justmake_absolute("dir"). This matches clang's behavior (default prefix is empty), but a brief comment could clarify this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/search_config.cpp` around lines 59 - 67, The code paths handling ID::OPT_iwithprefix and ID::OPT_iwithprefixbefore rely on the variable prefix set by ID::OPT_iprefix and currently concatenate prefix + arg->getValue() even when prefix is empty; add a concise inline comment above those cases (referencing OPT_iwithprefix, OPT_iwithprefixbefore, OPT_iprefix, prefix, make_absolute, after, and angled) stating that an absent -iprefix leaves prefix empty and thus make_absolute(prefix + arg->getValue()) yields make_absolute("dir"), matching clang's semantics and is intentional to aid future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/command/command.cpp`:
- Around line 791-800: The current replacement loop in command.cpp uses
llvm::StringRef::starts_with(old_resource_dir) and can false-match prefixes
(e.g., "/usr/lib" matching "/usr/lib64"); update the check to ensure a path
boundary after the prefix: for each argument (arguments, s) only perform the
replacement when s == old_resource_dir or when s.starts_with(old_resource_dir)
&& (s.size() == old_resource_dir.size() || s[old_resource_dir.size()] == '/' /*
or platform path separator */); keep the rest of the replacement logic
(construct replaced via resource_dir(), save with self->strings.save, assign to
arg) unchanged.
In `@src/command/toolchain_provider.cpp`:
- Around line 56-69: The function extract_toolchain_flags currently reads
arguments[0] without checking that arguments is non-empty, which can crash when
query_toolchain_cached calls it; add a precondition guard at the start of
extract_toolchain_flags to verify arguments.empty() is false (or assert/return
an empty ToolchainExtract) and handle the empty case safely so subsequent uses
of arguments[0] (and push_back(arguments[0])) are only executed when arguments
has at least one element; update query_toolchain_cached/get_pending_queries call
sites if needed to propagate or handle the empty result consistently.
In `@src/support/path_pool.h`:
- Around line 19-35: The intern/resolve logic needs defensive checks: in
intern(llvm::StringRef path) ensure the conversion of paths.size() to
std::uint32_t cannot overflow by checking paths.size() <=
std::numeric_limits<std::uint32_t>::max() before using it as the value for
cache.try_emplace (and handle the overflow case deterministically, e.g., return
an error/throw/abort); likewise in resolve(std::uint32_t id) add a bounds check
that id < paths.size() (and handle out-of-range ids deterministically, e.g.,
assert/throw/return an empty StringRef), and make sure these checks reference
the existing symbols (intern, resolve, cache, paths, allocator) so indexing into
paths and the uint32_t conversion are safe.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Line 17: Update the malformed comments in CMakeLists.txt: change "#Make sure
all third libraries are affected by ABI related options" to include a space
after the hash ("# Make sure..."), fix the URL comment at the location with
"https: // conda-forge" to "# https://conda-forge", correct the malformed
linker flag comment from "#, / OPT : NOICF" to a proper form like "#
/OPT:NOICF", and add a space for "# Temporary" instead of "#Temporary"; locate
these comments by their current text snippets to apply the exact replacements.
In `@src/clice.cc`:
- Around line 77-78: The code does not handle the case where
llvm::sys::fs::getMainExecutable returns an empty self_path; update the logic
around the static int anchor and std::string self_path to check if
self_path.empty() after calling getMainExecutable and, if empty, log an error
(e.g., via existing logging mechanism) and abort initialization or return a
non-zero exit code instead of passing an invalid path into MasterServer; ensure
the error message clearly mentions getMainExecutable() failure and that it
prevents resource directory initialization so users see the failure.
In `@src/command/command.cpp`:
- Around line 161-163: The helper options_bits currently encodes only the
query_toolchain flag and lacks documentation about future extensibility; add a
short comment above the static std::uint8_t options_bits(const CommandOptions&
options) function to state that it returns a bitmask used by SearchConfig, list
that bit 0 represents query_toolchain, and instruct maintainers to update this
bitmask and corresponding SearchConfig logic whenever new CommandOptions fields
that influence search results are added (or consider switching to an enum-based
bitset if many flags are expected).
In `@src/command/search_config.cpp`:
- Around line 59-67: The code paths handling ID::OPT_iwithprefix and
ID::OPT_iwithprefixbefore rely on the variable prefix set by ID::OPT_iprefix and
currently concatenate prefix + arg->getValue() even when prefix is empty; add a
concise inline comment above those cases (referencing OPT_iwithprefix,
OPT_iwithprefixbefore, OPT_iprefix, prefix, make_absolute, after, and angled)
stating that an absent -iprefix leaves prefix empty and thus
make_absolute(prefix + arg->getValue()) yields make_absolute("dir"), matching
clang's semantics and is intentional to aid future readers.
In `@src/command/toolchain_provider.h`:
- Around line 14-20: ToolchainQuery's file and directory members are
llvm::StringRef (non-owning) so their backing data must outlive the query;
update the declaration or docs to make this explicit: either change
ToolchainQuery::file and ToolchainQuery::directory to std::string to own the
data (and adjust constructors/uses like wherever ToolchainQuery is constructed)
or add a clear comment above struct ToolchainQuery documenting the lifetime
requirement that callers must ensure the referenced strings remain valid until
execution (and audit call sites that enqueue queries across async boundaries to
copy into std::string if needed).
In `@src/support/path_pool.h`:
- Around line 14-17: PathPool currently exposes allocator, paths, and cache as
public members which allows external code to mutate them and break the invariant
that cache[id] corresponds to paths[id]; make these three members private in
struct PathPool and provide a minimal API (e.g.,
addPath/lookupOrAdd/getPathById/clear/removeLast) that performs all mutations
and keeps cache and paths in sync (use allocator for storage when creating
StringRef entries, update llvm::SmallVector paths and llvm::StringMap cache
together inside these methods). Update all callers (e.g., uses in
project_index.cpp) to use the new API instead of touching allocator/paths/cache
directly so invariants are preserved.
In `@tests/unit/command/toolchain_provider_tests.cpp`:
- Around line 28-45: Add an assertion that verifies which value is retained when
duplicate keys are injected: after calling ToolchainProvider::inject_results
with two ToolchainResult entries having the same key ("key1"), call
ToolchainProvider::query_cached (or the appropriate lookup method) for "key1"
and assert that the returned ToolchainResult matches the first injected entry
(the one with "-triple", "x86_64"); reference ToolchainProvider, inject_results,
query_cached, and ToolchainResult to locate where to add this check in the
TEST_CASE.
In `@tests/unit/test/temp_dir.h`:
- Around line 59-67: touch() silently ignores file-creation errors from
llvm::raw_fd_ostream which leads to confusing test failures; update touch (and
its use of path() and llvm::sys::fs::create_directories) to check the
std::error_code ec after constructing llvm::raw_fd_ostream and fail fast when ec
indicates an error—either assert/llvm_unreachable or throw a runtime_error (with
the ec.message() included) so tests immediately surface file creation problems
instead of silently returning.
- Around line 26-28: The TempDir constructor currently calls
llvm::sys::fs::createUniqueDirectory(prefix, root) and ignores its return value;
check the returned llvm::ErrorOr/std::error_code and handle failures (e.g.,
throw a runtime_error, assert, or propagate the error) instead of proceeding
with an invalid root. Update the TempDir constructor to inspect the result of
createUniqueDirectory, log or propagate the error on failure, and ensure root is
only used/set when creation succeeds (refer to the TempDir constructor,
createUniqueDirectory call, and the root member when implementing the check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0654e1e7-4afe-45b2-a7d2-48fa8fc8dab5
📒 Files selected for processing (21)
CMakeLists.txtsrc/clice.ccsrc/command/command.cppsrc/command/command.hsrc/command/search_config.cppsrc/command/search_config.hsrc/command/toolchain_provider.cppsrc/command/toolchain_provider.hsrc/server/master_server.cppsrc/support/filesystem.hsrc/support/path_pool.htests/unit/command/command_tests.cpptests/unit/command/search_config_tests.cpptests/unit/command/toolchain_provider_tests.cpptests/unit/command/toolchain_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.htests/unit/test/temp_dir.htests/unit/test/tester.cpptests/unit/unit_tests.cc
💤 Files with no reviewable changes (3)
- tests/unit/test/tester.cpp
- src/support/filesystem.h
- tests/unit/unit_tests.cc
16bit-ykiko
force-pushed
the
feat/search-config-v2
branch
from
March 25, 2026 18:50
a365580 to
555fb62
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/command/toolchain_provider.cpp (1)
123-131:⚠️ Potential issue | 🟠 MajorAdd precondition check to prevent crash on empty arguments.
extract_toolchain_flagsaccessesarguments[0](lines 62, 69) without bounds checking. Whileget_pending_querieshas a guard at line 169,query_toolchain_cacheddoes not. This inconsistency creates a maintenance risk where callers might pass empty arguments.🛡️ Proposed fix to add precondition check
llvm::ArrayRef<const char*> query_toolchain_cached(this Impl& self, llvm::StringRef file, llvm::StringRef directory, llvm::ArrayRef<const char*> arguments) { + if(arguments.empty()) { + return {}; + } auto [key, query_args] = self.extract_toolchain_flags(file, arguments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/toolchain_provider.cpp` around lines 123 - 131, query_toolchain_cached calls extract_toolchain_flags which indexes arguments[0] and can crash on empty input; add a precondition at the top of query_toolchain_cached to check arguments.empty() and handle it safely (for example return an empty llvm::ArrayRef<const char*> or propagate an error/ASSERT) before calling extract_toolchain_flags; reference the functions query_toolchain_cached and extract_toolchain_flags and mirror the guard logic used in get_pending_queries to maintain consistent behavior.
🧹 Nitpick comments (3)
src/command/toolchain_provider.cpp (1)
191-203: Minor simplification opportunity ininject_results.The
count()check beforetry_emplaceis redundant sincetry_emplacealready handles existing keys by not inserting. However, the current code is correct and the explicit check makes the intent clearer.♻️ Optional simplification
void ToolchainProvider::inject_results(llvm::ArrayRef<ToolchainResult> results) { for(auto& result: results) { - if(self->toolchain_cache.count(result.key)) { - continue; - } std::vector<const char*> saved; saved.reserve(result.cc1_args.size()); for(auto& arg: result.cc1_args) { saved.push_back(self->strings.save(arg).data()); } - self->toolchain_cache.try_emplace(result.key, std::move(saved)); + // try_emplace is a no-op if key exists + self->toolchain_cache.try_emplace(std::move(result.key), std::move(saved)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/toolchain_provider.cpp` around lines 191 - 203, The pre-check using self->toolchain_cache.count(result.key) before calling try_emplace in ToolchainProvider::inject_results is redundant; replace that pattern by first doing a lookup once and skipping when present (auto it = self->toolchain_cache.find(result.key); if (it != self->toolchain_cache.end()) continue;) and then build the saved vector and call self->toolchain_cache.try_emplace(result.key, std::move(saved)); ensuring you only construct saved when you actually intend to insert.tests/unit/command/toolchain_provider_tests.cpp (2)
28-45: The duplicate-key test doesn't prove the dedup policy.
EXPECT_TRUE(provider.has_cached_entries())on Line 44 passes whether the second"key1"is ignored or overwrites the first. Build aPendingEntryfor that key and assert whatquery_cached()returns so this test actually fixes the collision semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/command/toolchain_provider_tests.cpp` around lines 28 - 45, The test only checks provider.has_cached_entries() which doesn't prove dedup behavior; update InjectResultsSkipsDuplicateKeys to build a PendingEntry for "key1" and call provider.query_cached("key1", pending) and assert that the returned PendingEntry matches the first injected result (the args {"-cc1","-triple","x86_64"}) to ensure the second injection did not overwrite the original; reference ToolchainProvider::inject_results, ToolchainProvider::query_cached, and the PendingEntry structure and assert on the stored args (or equality of PendingEntry) rather than only has_cached_entries().
47-74: This "uncached only" test never preloads the key it later queries.The literal
"preloaded_key"on Line 53 cannot match the derived key for theclang++/.cppentries on Lines 60-68, soEXPECT_EQ(queries.size(), 1u)just re-tests same-key dedup. Reuse a key discovered from an initialget_pending_queries()call, like the pattern inInjectThenGetPendingSkipsCached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/command/toolchain_provider_tests.cpp` around lines 47 - 74, The test injects a literal "preloaded_key" that never matches the derived key for the clang++/.cpp entries; to fix, first call ToolchainProvider::get_pending_queries(...) with the constructed PendingEntry list to obtain the actual generated query key(s), then call provider.inject_results(...) using that returned key (or build a ToolchainResult using the query's key) before re-calling get_pending_queries; update the test to mirror the pattern used in InjectThenGetPendingSkipsCached (use provider.get_pending_queries to get the real key, then provider.inject_results with that key) so the cached entry is recognized and the expectation (queries.size()) reflects the uncached-only behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/command/command.cpp`:
- Around line 156-164: The cache key for search_config_cache (ConfigCacheKey)
only includes options_bits from options.query_toolchain, but
lookup_search_config() and mangle_command() also depend on
options.append/options.remove and the current toolchain state (self->toolchain),
so stale SearchConfig entries can be returned; fix by broadening the cache key
to include the append/remove option bits (extend options_bits to encode
options.append/options.remove) and include a toolchain identity/version (e.g.,
pointer or generation id from self->toolchain) or else clear/invalidate
search_config_cache when inject_results() or any toolchain update occurs so
lookups in lookup_search_config() always reflect mangle_command() and current
toolchain.
- Around line 763-818: The code currently replaces arguments with the cc1 result
from self->toolchain.query_cached(file, directory, user_args), losing user-level
flags like -D/-U/-include/-idirafter/-nostdinc; instead, preserve user_args and
merge the toolchain-derived system/driver args into it. Concretely: after
getting cached (result of query_cached) parse cached into a temporary vector
(remove the temp source file and apply the resource_dir() replacement on that
cached vector as the existing loop does), then walk cached and insert only the
system/driver arguments (system include flags, -resource-dir, driver-only flags)
into user_args (or append them in appropriate positions) rather than doing
arguments.assign(cached.begin(), cached.end()); finally set arguments =
std::move(user_args) so parser.parse/append_arg still replays user flags like
-D/-U/-include/-idirafter/-nostdinc while keeping the toolchain's system paths.
Reference: query_cached, user_args, arguments, resource_dir(), parser.parse,
append_arg.
In `@tests/unit/test/temp_dir.h`:
- Around line 26-32: The TempDir constructor and destructor currently ignore the
llvm::sys::fs return values; modify the TempDir(llvm::StringRef prefix)
constructor to capture and check the result of createUniqueDirectory(prefix,
root) and handle failure (e.g., assert, llvm::report_fatal_error, or throw) so
tests fail loudly if the directory isn't created, and similarly check the return
from remove_directories(root) in ~TempDir() and handle/report errors (or at
minimum assert success); reference the TempDir class, its
constructor/destructor, and the functions createUniqueDirectory and
remove_directories operating on the member root when adding these checks.
- Around line 59-67: The touch() helper in temp_dir.h silently ignores failures
when creating/writing the file (it constructs llvm::raw_fd_ostream with
std::error_code ec and returns if ec), which can mask test infra problems;
update touch() (referencing touch(), path(), llvm::sys::fs::create_directories,
llvm::raw_fd_ostream and the std::error_code ec) to check ec after constructing
the stream and after any write/close and either assert/fail the test or log the
full error (include ec.message() and the target path) so file creation/writes do
not silently fail.
---
Duplicate comments:
In `@src/command/toolchain_provider.cpp`:
- Around line 123-131: query_toolchain_cached calls extract_toolchain_flags
which indexes arguments[0] and can crash on empty input; add a precondition at
the top of query_toolchain_cached to check arguments.empty() and handle it
safely (for example return an empty llvm::ArrayRef<const char*> or propagate an
error/ASSERT) before calling extract_toolchain_flags; reference the functions
query_toolchain_cached and extract_toolchain_flags and mirror the guard logic
used in get_pending_queries to maintain consistent behavior.
---
Nitpick comments:
In `@src/command/toolchain_provider.cpp`:
- Around line 191-203: The pre-check using
self->toolchain_cache.count(result.key) before calling try_emplace in
ToolchainProvider::inject_results is redundant; replace that pattern by first
doing a lookup once and skipping when present (auto it =
self->toolchain_cache.find(result.key); if (it != self->toolchain_cache.end())
continue;) and then build the saved vector and call
self->toolchain_cache.try_emplace(result.key, std::move(saved)); ensuring you
only construct saved when you actually intend to insert.
In `@tests/unit/command/toolchain_provider_tests.cpp`:
- Around line 28-45: The test only checks provider.has_cached_entries() which
doesn't prove dedup behavior; update InjectResultsSkipsDuplicateKeys to build a
PendingEntry for "key1" and call provider.query_cached("key1", pending) and
assert that the returned PendingEntry matches the first injected result (the
args {"-cc1","-triple","x86_64"}) to ensure the second injection did not
overwrite the original; reference ToolchainProvider::inject_results,
ToolchainProvider::query_cached, and the PendingEntry structure and assert on
the stored args (or equality of PendingEntry) rather than only
has_cached_entries().
- Around line 47-74: The test injects a literal "preloaded_key" that never
matches the derived key for the clang++/.cpp entries; to fix, first call
ToolchainProvider::get_pending_queries(...) with the constructed PendingEntry
list to obtain the actual generated query key(s), then call
provider.inject_results(...) using that returned key (or build a ToolchainResult
using the query's key) before re-calling get_pending_queries; update the test to
mirror the pattern used in InjectThenGetPendingSkipsCached (use
provider.get_pending_queries to get the real key, then provider.inject_results
with that key) so the cached entry is recognized and the expectation
(queries.size()) reflects the uncached-only behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c94dc942-1526-48c3-9439-e33b56131e31
📒 Files selected for processing (21)
CMakeLists.txtsrc/clice.ccsrc/command/command.cppsrc/command/command.hsrc/command/search_config.cppsrc/command/search_config.hsrc/command/toolchain_provider.cppsrc/command/toolchain_provider.hsrc/server/master_server.cppsrc/support/filesystem.hsrc/support/path_pool.htests/unit/command/command_tests.cpptests/unit/command/search_config_tests.cpptests/unit/command/toolchain_provider_tests.cpptests/unit/command/toolchain_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.htests/unit/test/temp_dir.htests/unit/test/tester.cpptests/unit/unit_tests.cc
💤 Files with no reviewable changes (3)
- tests/unit/test/tester.cpp
- src/support/filesystem.h
- tests/unit/unit_tests.cc
✅ Files skipped from review due to trivial changes (2)
- tests/unit/feature/inlay_hint_tests.cpp
- src/command/search_config.h
🚧 Files skipped from review as they are similar to previous changes (6)
- src/server/master_server.cpp
- src/clice.cc
- tests/unit/command/command_tests.cpp
- src/support/path_pool.h
- src/command/search_config.cpp
- src/command/toolchain_provider.h
- SearchConfig: extract header search directories from compilation args in four-segment model matching clang's InitHeaderSearch::Realize layout - ToolchainProvider: manage toolchain query caching with canonical keys, batch pre-warming support, and deduplication - PathPool: intern pool mapping file paths to compact uint32_t IDs - TempDir: RAII test helper for temporary directory trees - Integrate ToolchainProvider into CompilationDatabase::lookup() for cached toolchain queries with user include path injection - Add SearchConfig cache to CompilationDatabase with options_bits key - Fix Quoted dedup bug: don't seed seen set with Quoted paths - Include -x language mode in toolchain cache key Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/search-config-v2
branch
from
March 26, 2026 02:42
555fb62 to
3d67f4c
Compare
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)
src/command/command.cpp (1)
728-728:⚠️ Potential issue | 🟡 Minor
[[unlikely]]attribute appears incorrect.Finding a file in the compilation database should be the common/expected case, not unlikely. This attribute may cause the compiler to pessimize the hot path.
🔧 Suggested fix
- if(it != self->files.end()) [[unlikely]] { + if(it != self->files.end()) [[likely]] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` at line 728, The branch attribute on the if-condition is inverted: the check "if (it != self->files.end())" is the expected/common path, so remove the "[[unlikely]]" attribute (or change it to "[[likely]]") on that if-statement to avoid pessimizing the hot path; update the condition at the "if(it != self->files.end()) [[unlikely]]" site to simply "if (it != self->files.end())" (or "if (it != self->files.end()) [[likely]]") to reflect the common case.
♻️ Duplicate comments (3)
src/command/command.cpp (2)
793-802:⚠️ Potential issue | 🟡 MinorResource directory replacement may have false positives with prefix matching.
The replacement logic replaces any argument starting with
old_resource_dir, which could incorrectly match unrelated paths ifold_resource_diris a prefix (e.g.,/usr/libmatching/usr/lib64/something). This was flagged in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 793 - 802, The prefix check on arguments may match partial path components (e.g., "/usr/lib" vs "/usr/lib64"); modify the replacement in the loop over arguments so it only treats an argument as starting with old_resource_dir when the match is a full path component boundary: keep the existing llvm::StringRef s and replace only if s.startswith(old_resource_dir) AND (s.size() == old_resource_dir.size() OR s[old_resource_dir.size()] is the path separator '/'), or normalize old_resource_dir to always include a trailing '/' and require the startswith against that normalized value; update the code surrounding old_resource_dir, resource_dir(), arguments and the replacement that uses self->strings.save to use this stricter condition.
156-164:⚠️ Potential issue | 🟠 MajorCache key may be insufficient for options.append/remove variations.
The
options_bitsonly encodesquery_toolchain, butlookup_search_configcallslookup()which usesmangle_command(). Ifoptions.appendoroptions.removecontain include-path flags (-I,-isystem,-iquote), the resultingSearchConfigwould differ, but they'd share the same cache key.This was flagged in a previous review. Consider extending
options_bitsor documenting that append/remove must not contain include-related flags when using cached configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 156 - 164, The cache key (ConfigCacheKey) currently only encodes query_toolchain via options_bits, but lookup_search_config and lookup() ultimately call mangle_command() so differences in options.append/remove (especially include-path flags -I, -isystem, -iquote) can change the produced SearchConfig; update options_bits(const CommandOptions&) to incorporate whether options.append or options.remove contain include-related flags or, better, a small stable hash/bitmask representing their include-path affecting entries (or expand the key type from uint8_t to a larger integer) so the DenseMap key distinguishes commands with different append/remove include flags; alternatively, document/prohibit use of include flags in append/remove when relying on the cache and enforce that in lookup_search_config.src/command/toolchain_provider.cpp (1)
56-69:⚠️ Potential issue | 🟡 MinorAdd precondition check for empty arguments in
query_toolchain_cached.
extract_toolchain_flagsaccessesarguments[0](lines 62, 69) without bounds checking. Whileget_pending_querieshas a guard at line 169,query_toolchain_cached(which also callsextract_toolchain_flags) lacks this protection. This was flagged in a previous review.🛡️ Proposed fix
llvm::ArrayRef<const char*> query_toolchain_cached(this Impl& self, llvm::StringRef file, llvm::StringRef directory, llvm::ArrayRef<const char*> arguments) { + if(arguments.empty()) { + return {}; + } auto [key, query_args] = self.extract_toolchain_flags(file, arguments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/toolchain_provider.cpp` around lines 56 - 69, The code calls extract_toolchain_flags which reads arguments[0] without checking bounds; add a precondition check in query_toolchain_cached to ensure the arguments array is non-empty before calling extract_toolchain_flags (mirror the guard used in get_pending_queries), and handle the empty-case by returning early (e.g., an error/empty result) or logging and skipping the query; optionally add an assert or explicit check at the start of extract_toolchain_flags to document the requirement (referencing extract_toolchain_flags, query_toolchain_cached, get_pending_queries, and the use of arguments[0]).
🧹 Nitpick comments (2)
tests/unit/command/search_config_tests.cpp (1)
28-37: Consider assertingafter_start_idxfor completeness.This test validates the four-segment model but only asserts
angled_start_idxandsystem_start_idx. For consistency with other tests and to fully verify the segment boundaries, consider adding an assertion forafter_start_idx.💡 Suggested addition
EXPECT_EQ(config.angled_start_idx, 1u); EXPECT_EQ(config.system_start_idx, 2u); + EXPECT_EQ(config.after_start_idx, 5u); // No -idirafter dirs, so after == end EXPECT_EQ(config.dirs[0].path, tmp.path("quoted"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/command/search_config_tests.cpp` around lines 28 - 37, Add an assertion for the after_start_idx boundary in the test to fully validate the four-segment model: after confirming config.dirs size and the existing angled_start_idx and system_start_idx checks, assert that config.after_start_idx equals the expected index (e.g., 4u) so the test verifies the quoted/user/stdlib/clang/sysroot split; update the test in search_config_tests.cpp near the existing EXPECT_EQ checks for angled_start_idx and system_start_idx to include this new EXPECT_EQ(config.after_start_idx, ...).src/command/toolchain_provider.h (1)
14-20: DocumentStringReflifetime requirements forToolchainQuery.
ToolchainQuery::fileandToolchainQuery::directoryarellvm::StringRefwhich don't own their data. The caller must ensure the underlying strings outlive theToolchainQueryobjects. Consider documenting this requirement or usingstd::stringif ownership is intended.📝 Suggested documentation
/// A pending toolchain query, ready to be executed (possibly in parallel). +/// Note: `file` and `directory` are non-owning StringRefs. The caller must +/// ensure the underlying strings outlive the ToolchainQuery. struct ToolchainQuery { std::string key; std::vector<const char*> query_args;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/toolchain_provider.h` around lines 14 - 20, ToolchainQuery::file and ToolchainQuery::directory are llvm::StringRef (non-owning) so callers must guarantee the referenced storage outlives each ToolchainQuery; either add a clear comment above struct ToolchainQuery documenting this lifetime requirement for file and directory, or change those members to std::string to take ownership (update constructors/usages such as ToolchainQuery instances wherever created). Ensure you reference ToolchainQuery::file and ToolchainQuery::directory in the comment and update any call sites that create ToolchainQuery to avoid dangling references if you choose the StringRef approach.
🤖 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/unit/command/toolchain_provider_tests.cpp`:
- Around line 47-74: The test's injected result uses a literal key
"preloaded_key" that will never match keys produced by
ToolchainProvider::extract_toolchain_flags, so update the test to reflect the
real behavior: either rename the test from GetPendingQueriesReturnsUncachedOnly
to GetPendingQueriesDeduplicatesSameKeyFromMultipleEntries to indicate it
verifies deduplication of identical generated keys, or rewrite it to follow the
InjectThenGetPendingSkipsCached pattern used elsewhere (see tests around
InjectThenGetPendingSkipsCached) by calling provider.inject_results with the
actual generated key (obtained via ToolchainProvider::extract_toolchain_flags or
by first creating a query and capturing its key) before calling
ToolchainProvider::get_pending_queries so that the injected cached result will
be correctly recognized and skipped.
---
Outside diff comments:
In `@src/command/command.cpp`:
- Line 728: The branch attribute on the if-condition is inverted: the check "if
(it != self->files.end())" is the expected/common path, so remove the
"[[unlikely]]" attribute (or change it to "[[likely]]") on that if-statement to
avoid pessimizing the hot path; update the condition at the "if(it !=
self->files.end()) [[unlikely]]" site to simply "if (it != self->files.end())"
(or "if (it != self->files.end()) [[likely]]") to reflect the common case.
---
Duplicate comments:
In `@src/command/command.cpp`:
- Around line 793-802: The prefix check on arguments may match partial path
components (e.g., "/usr/lib" vs "/usr/lib64"); modify the replacement in the
loop over arguments so it only treats an argument as starting with
old_resource_dir when the match is a full path component boundary: keep the
existing llvm::StringRef s and replace only if s.startswith(old_resource_dir)
AND (s.size() == old_resource_dir.size() OR s[old_resource_dir.size()] is the
path separator '/'), or normalize old_resource_dir to always include a trailing
'/' and require the startswith against that normalized value; update the code
surrounding old_resource_dir, resource_dir(), arguments and the replacement that
uses self->strings.save to use this stricter condition.
- Around line 156-164: The cache key (ConfigCacheKey) currently only encodes
query_toolchain via options_bits, but lookup_search_config and lookup()
ultimately call mangle_command() so differences in options.append/remove
(especially include-path flags -I, -isystem, -iquote) can change the produced
SearchConfig; update options_bits(const CommandOptions&) to incorporate whether
options.append or options.remove contain include-related flags or, better, a
small stable hash/bitmask representing their include-path affecting entries (or
expand the key type from uint8_t to a larger integer) so the DenseMap key
distinguishes commands with different append/remove include flags;
alternatively, document/prohibit use of include flags in append/remove when
relying on the cache and enforce that in lookup_search_config.
In `@src/command/toolchain_provider.cpp`:
- Around line 56-69: The code calls extract_toolchain_flags which reads
arguments[0] without checking bounds; add a precondition check in
query_toolchain_cached to ensure the arguments array is non-empty before calling
extract_toolchain_flags (mirror the guard used in get_pending_queries), and
handle the empty-case by returning early (e.g., an error/empty result) or
logging and skipping the query; optionally add an assert or explicit check at
the start of extract_toolchain_flags to document the requirement (referencing
extract_toolchain_flags, query_toolchain_cached, get_pending_queries, and the
use of arguments[0]).
---
Nitpick comments:
In `@src/command/toolchain_provider.h`:
- Around line 14-20: ToolchainQuery::file and ToolchainQuery::directory are
llvm::StringRef (non-owning) so callers must guarantee the referenced storage
outlives each ToolchainQuery; either add a clear comment above struct
ToolchainQuery documenting this lifetime requirement for file and directory, or
change those members to std::string to take ownership (update
constructors/usages such as ToolchainQuery instances wherever created). Ensure
you reference ToolchainQuery::file and ToolchainQuery::directory in the comment
and update any call sites that create ToolchainQuery to avoid dangling
references if you choose the StringRef approach.
In `@tests/unit/command/search_config_tests.cpp`:
- Around line 28-37: Add an assertion for the after_start_idx boundary in the
test to fully validate the four-segment model: after confirming config.dirs size
and the existing angled_start_idx and system_start_idx checks, assert that
config.after_start_idx equals the expected index (e.g., 4u) so the test verifies
the quoted/user/stdlib/clang/sysroot split; update the test in
search_config_tests.cpp near the existing EXPECT_EQ checks for angled_start_idx
and system_start_idx to include this new EXPECT_EQ(config.after_start_idx, ...).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8ad7947-e5f0-4f27-a52a-a653aa7d0af8
📒 Files selected for processing (11)
CMakeLists.txtsrc/command/command.cppsrc/command/command.hsrc/command/search_config.cppsrc/command/search_config.hsrc/command/toolchain_provider.cppsrc/command/toolchain_provider.hsrc/support/path_pool.htests/unit/command/search_config_tests.cpptests/unit/command/toolchain_provider_tests.cpptests/unit/test/temp_dir.h
✅ Files skipped from review due to trivial changes (2)
- CMakeLists.txt
- src/command/search_config.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/support/path_pool.h
- src/command/search_config.cpp
…in path The toolchain query replaces the entire argument list with cc1 results, but only -I/-isystem/-iquote were being replayed from user args. This lost preprocessor defines (-D/-U), forced includes (-include), and fallback include dirs (-idirafter). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oach Instead of only including specific toolchain options in the cache key, now all options are included except user-content options (-I/-D/-U/ -include/-idirafter/-isystem/-iquote and INPUT). This ensures the cc1 result correctly reflects compiler semantics (-f/-W/-O flags etc.) without needing to replay them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/command/toolchain_provider_tests.cpp (1)
15-45: Consider strengthening the duplicate key test with explicit verification.
InjectResultsSkipsDuplicateKeysonly verifies that the cache is populated (has_cached_entries()), but doesn't confirm which result wins when duplicate keys are injected. A stronger test would verify that the first result is preserved (or document the expected behavior).// After injection, could verify the actual cached result: // auto cached = provider.query_cached("test.cpp", "/tmp", {"clang++", "test.cpp"}); // EXPECT_EQ(cached contains "x86_64");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/command/toolchain_provider_tests.cpp` around lines 15 - 45, Update the InjectResultsSkipsDuplicateKeys test to explicitly assert which ToolchainResult is stored after injecting duplicates: call provider.query_cached(...) (or the equivalent lookup API on ToolchainProvider) for the key "key1" after provider.inject_results(...) and ASSERT/EXPECT that the cached command list contains the triple "x86_64" (the first injected value) to verify the first result is preserved; keep the existing has_cached_entries() check but add this explicit equality/contains assertion to make the test deterministic about duplicate-key behavior.src/command/command.cpp (1)
869-907: Consider extracting duplicate context-resolution logic.Lines 876-888 duplicate the
CompilationInfolookup logic fromlookup()(lines 728-742). A private helper method likeresolve_compilation_info(file, context)would reduce duplication and maintenance burden.♻️ Example refactor
// In Impl struct: object_ptr<CompilationInfo> resolve_info(this Impl& self, StringID path_id, const void* context) { auto it = self.files.find(path_id); if(it == self.files.end()) return nullptr; if(!context) return it->second->info; for(auto cur = it->second; cur; cur = cur->next) { if(cur->info.ptr == context) return cur->info; } return nullptr; }Then use in both
lookup()andlookup_search_config().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 869 - 907, The context-resolution logic in lookup_search_config duplicates the CompilationInfo lookup from lookup; extract that logic into a private helper on Impl (e.g., resolve_info / resolve_compilation_info) that takes path_id (from self->strings.get(file)) and const void* context and returns the matching CompilationInfo* or nullptr, then replace the duplicated block in both lookup_search_config and lookup to call this helper; update uses of it->second->info.ptr to use the helper's result and keep the existing caching/key logic (Impl::ConfigCacheKey, Impl::options_bits) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/command/command.cpp`:
- Around line 763-779: The code calls arguments.pop_back() after assigning
cached into arguments without verifying cached.size() > 0; add a defensive
bounds check before popping: after auto cached =
self->toolchain.query_cached(file, directory, user_args); and before
arguments.assign(...) / pop_back(), ensure cached.size() >= 1 (or check
arguments.empty() after assign) and handle the unexpected empty case by logging
an error/warning and restoring arguments = std::move(user_args) (or skipping
pop_back) so pop_back() is never invoked on an empty container; update the logic
around toolchain.query_cached, arguments.assign, and arguments.pop_back
accordingly.
---
Nitpick comments:
In `@src/command/command.cpp`:
- Around line 869-907: The context-resolution logic in lookup_search_config
duplicates the CompilationInfo lookup from lookup; extract that logic into a
private helper on Impl (e.g., resolve_info / resolve_compilation_info) that
takes path_id (from self->strings.get(file)) and const void* context and returns
the matching CompilationInfo* or nullptr, then replace the duplicated block in
both lookup_search_config and lookup to call this helper; update uses of
it->second->info.ptr to use the helper's result and keep the existing
caching/key logic (Impl::ConfigCacheKey, Impl::options_bits) unchanged.
In `@tests/unit/command/toolchain_provider_tests.cpp`:
- Around line 15-45: Update the InjectResultsSkipsDuplicateKeys test to
explicitly assert which ToolchainResult is stored after injecting duplicates:
call provider.query_cached(...) (or the equivalent lookup API on
ToolchainProvider) for the key "key1" after provider.inject_results(...) and
ASSERT/EXPECT that the cached command list contains the triple "x86_64" (the
first injected value) to verify the first result is preserved; keep the existing
has_cached_entries() check but add this explicit equality/contains assertion to
make the test deterministic about duplicate-key behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf040e75-4057-4b1a-bbb5-238667b34b01
📒 Files selected for processing (3)
src/command/command.cppsrc/command/toolchain_provider.cpptests/unit/command/toolchain_provider_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/command/toolchain_provider.cpp
Strip backend/linker-only flags (-fPIC, -fomit-frame-pointer, -funwind-tables, -fstack-protector, -flto, -fdata-sections, -fdiagnostics-color, -ftrapping-math, debug info group, etc.) early in the CDB pipeline. These don't affect frontend semantics and are irrelevant to an LSP server. Options that DO affect semantics (-fno-exceptions, -fno-rtti, -std=*, -march=*, -fsanitize=*, -O*, -W*) are intentionally kept. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore CMakeLists.txt comments corrupted by formatter - Fix misleading on_error comment in extract_toolchain_flags (unknown args are dropped, not included in key) - Make ToolchainQuery::file/directory owning std::string to prevent potential dangling StringRef - Add bounds assertion to PathPool::resolve Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 27, 2026
This was referenced Apr 6, 2026
3 tasks done
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
InitHeaderSearch::Realizelayout-xlanguage mode in cache keyuint32_tIDsCompilationDatabase::lookup()for cached toolchain queriesRemoveDuplicatesbehavior)Depends on #369
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests