Repository navigation
feat: add SearchConfig, ToolchainProvider, PathPool - #367
Closed
16bit-ykiko wants to merge 2 commits into
Closed
16bit-ykiko wants to merge 2 commits into
16bit-ykiko wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new toolchain-provider abstraction with search-config extraction and caching to replace the global resource-directory mechanism. It removes the filesystem module's Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
Introduce the search-config extraction pipeline that models clang's four-segment include directory layout (Quoted → Angled → System → After), a ToolchainProvider cache for `clang++ -###` queries, and a PathPool intern table for lightweight file-path identity. Also adds a shared TempDir test helper and ExtractSearchConfig unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/search-config
branch
from
March 25, 2026 15:57
b161085 to
73bc73b
Compare
![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
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)
755-818:⚠️ Potential issue | 🔴 CriticalPreserve the original frontend flags on toolchain cache hits.
Lines 764-818 rebuild
argumentsfrom the minimal-###probe result and only re-add-I/-isystem/-iquote. That drops every other user-facing flag omitted from the probe (-D/-U/-W, warning suppressions, and the caller-supplied-resource-diron non-Windows) fromlookup().src/server/master_server.cpp:1-50consumesctx.argumentsdirectly afterlookup(..., {.resource_dir = true, .query_toolchain = true}), so warm-cache lookups can produce a materially different command line than cold-cache lookups.Please merge the discovered toolchain entries into
user_argsinstead of replacing the full argv, or re-inject all non-toolchain arguments before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 755 - 818, The current lookup path (involving toolchain.query_cached, the local variables arguments and user_args, and the parser.parse callback that only re-injects OPT_I/OPT_isystem/OPT_iquote) replaces the caller argv with the minimal cc1 probe result and thus drops user-facing flags (e.g. -D/-U/-W and caller -resource-dir); instead merge the cached toolchain args into the original user_args: after obtaining cached from self->toolchain.query_cached, iterate cached and for each token that is a toolchain-provided argument (system include paths, driver flags, etc.) insert or overwrite only those entries into user_args while preserving any original non-toolchain flags (macros, warning flags, user -resource-dir); finally assign arguments = std::move(user_args) (or reconstruct arguments by combining cached toolchain entries + original non-toolchain entries) and keep the existing Windows resource-dir replacement and include-path injection logic (use functions/variables: toolchain.query_cached, cached, user_args, arguments, append_arg, self->parser.parse to locate where to change).
🧹 Nitpick comments (1)
tests/unit/test/temp_dir.h (1)
59-67: Consider logging or propagating errors fromtouch().The
touch()method silently ignores file creation errors. Ifllvm::raw_fd_ostreamfails to open the file, tests may proceed with missing files, leading to confusing failures downstream.💡 Optional: Add error logging for test debugging
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) { + if(ec) { + llvm::errs() << "TempDir::touch failed for " << p << ": " << ec.message() << "\n"; + return; + } - out << content; - } + 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, The touch() helper currently swallows file creation errors; update it to detect and propagate or log the failure: after creating llvm::raw_fd_ostream(p, ec) check if ec is set and then either throw a std::runtime_error (including p and ec.message()) or emit a clear diagnostic via llvm::errs()/llvm::report_fatal_error so tests fail fast; reference the touch() function, the path() helper, llvm::raw_fd_ostream and the std::error_code ec when implementing the check and message.
🤖 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 155-158: The search_config_cache currently keys solely by
CompilationInfo* (search_config_cache) but lookup() also depends on
CommandOptions (query_toolchain, resource_dir, append, remove) and on
ToolchainProvider state changed by inject_results(), so update the cache key to
include those discriminating inputs (e.g., form a composite key struct
containing const CompilationInfo*, CommandOptions values that affect argv, and a
toolchain cache/version token from ToolchainProvider) or skip cached entries
when lookup() could mutate argv beyond the canonical CompilationInfo;
specifically modify lookup() and where search_config_cache is accessed/inserted
(symbols: search_config_cache, CompilationInfo, SearchConfig, lookup(),
CommandOptions.{query_toolchain,resource_dir,append,remove}, inject_results(),
ToolchainProvider) to ensure you either broaden the key or invalidate/bypass the
cache when toolchain warming or differing CommandOptions are in effect.
In `@src/command/toolchain_provider.cpp`:
- Around line 137-145: The code currently caches whatever
toolchain::query_toolchain(params) returns into self.toolchain_cache via
try_emplace(key, result), which persists empty vectors and prevents retries;
change the logic in the function that builds params and calls query_toolchain so
that after obtaining auto result you check if result is empty and only insert
into self.toolchain_cache when non-empty (i.e., skip try_emplace when
result.empty()), returning the fresh result directly in the empty case so
callers can retry later; reference symbols: toolchain::query_toolchain,
toolchain::QueryParams, result, key, self.toolchain_cache, try_emplace.
In `@src/support/path_pool.h`:
- Around line 14-36: PathPool currently hands out 0 as a valid ID and has no
bounds checking in resolve; change PathPool so ID 0 is reserved as a sentinel by
inserting an initial empty entry into paths (and ensure cache indices start from
1) before any intern() calls, adjust intern(llvm::StringRef path) to assign IDs
starting at 1 (e.g., use paths.size() after the sentinel) and keep existing
null-terminated allocation logic, and update resolve(std::uint32_t id) to check
that id != 0 and id < paths.size() returning an empty llvm::StringRef (or
assert/handle error) for invalid ids to avoid crashes.
---
Outside diff comments:
In `@src/command/command.cpp`:
- Around line 755-818: The current lookup path (involving
toolchain.query_cached, the local variables arguments and user_args, and the
parser.parse callback that only re-injects OPT_I/OPT_isystem/OPT_iquote)
replaces the caller argv with the minimal cc1 probe result and thus drops
user-facing flags (e.g. -D/-U/-W and caller -resource-dir); instead merge the
cached toolchain args into the original user_args: after obtaining cached from
self->toolchain.query_cached, iterate cached and for each token that is a
toolchain-provided argument (system include paths, driver flags, etc.) insert or
overwrite only those entries into user_args while preserving any original
non-toolchain flags (macros, warning flags, user -resource-dir); finally assign
arguments = std::move(user_args) (or reconstruct arguments by combining cached
toolchain entries + original non-toolchain entries) and keep the existing
Windows resource-dir replacement and include-path injection logic (use
functions/variables: toolchain.query_cached, cached, user_args, arguments,
append_arg, self->parser.parse to locate where to change).
---
Nitpick comments:
In `@tests/unit/test/temp_dir.h`:
- Around line 59-67: The touch() helper currently swallows file creation errors;
update it to detect and propagate or log the failure: after creating
llvm::raw_fd_ostream(p, ec) check if ec is set and then either throw a
std::runtime_error (including p and ec.message()) or emit a clear diagnostic via
llvm::errs()/llvm::report_fatal_error so tests fail fast; reference the touch()
function, the path() helper, llvm::raw_fd_ostream and the std::error_code ec
when implementing the check and message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a90da651-309b-4893-8efa-19008f664296
📒 Files selected for processing (10)
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/compile/command_tests.cpptests/unit/test/temp_dir.h
Comment on lines
+155
to
+158
| /// Cache of SearchConfig per CompilationInfo pointer. Since infos are | ||
| /// deduplicated by ObjectSet, the pointer uniquely identifies a compilation | ||
| /// context. This avoids re-parsing arguments on repeated scans. | ||
| llvm::DenseMap<const CompilationInfo*, SearchConfig> search_config_cache; |
There was a problem hiding this comment.
Key the SearchConfig cache on all inputs that affect lookup().
Lines 155-158 and 863-875 assume CompilationInfo* uniquely identifies the extracted search config, but lookup() also varies with CommandOptions (query_toolchain, resource_dir, append, remove) and with later ToolchainProvider warming via inject_results(). A first lookup on a cold toolchain cache can therefore memoize the fallback user_args config and keep returning it after system include paths become available, or after a caller requests a different option set.
Please widen the cache key or bypass this cache whenever lookup() can mutate the argv beyond the canonical CompilationInfo.
Also applies to: 841-875
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/command.cpp` around lines 155 - 158, The search_config_cache
currently keys solely by CompilationInfo* (search_config_cache) but lookup()
also depends on CommandOptions (query_toolchain, resource_dir, append, remove)
and on ToolchainProvider state changed by inject_results(), so update the cache
key to include those discriminating inputs (e.g., form a composite key struct
containing const CompilationInfo*, CommandOptions values that affect argv, and a
toolchain cache/version token from ToolchainProvider) or skip cached entries
when lookup() could mutate argv beyond the canonical CompilationInfo;
specifically modify lookup() and where search_config_cache is accessed/inserted
(symbols: search_config_cache, CompilationInfo, SearchConfig, lookup(),
CommandOptions.{query_toolchain,resource_dir,append,remove}, inject_results(),
ToolchainProvider) to ensure you either broaden the key or invalidate/bypass the
cache when toolchain warming or differing CommandOptions are in effect.
Comment on lines
+137
to
+145
| auto callback = [&](const char* s) -> const char* { | ||
| return self.strings.save(s).data(); | ||
| }; | ||
| toolchain::QueryParams params = {file, directory, query_args, callback}; | ||
| auto result = toolchain::query_toolchain(params); | ||
|
|
||
| auto [entry, _] = self.toolchain_cache.try_emplace(std::move(key), std::move(result)); | ||
| return entry->second; | ||
| } |
There was a problem hiding this comment.
Empty query results are cached, preventing retries on transient failures.
If toolchain::query_toolchain() returns an empty vector (e.g., driver not found, execution failure), this empty result is cached permanently. Subsequent lookups for the same toolchain key will return the empty result without retrying, even if the issue was transient (e.g., network file system glitch, temporary resource exhaustion).
Consider either:
- Not caching empty results to allow retries
- Adding a TTL or retry mechanism for failed queries
- Documenting this as intentional behavior (to avoid hammering broken toolchains)
💡 Option 1: Skip caching empty results
auto callback = [&](const char* s) -> const char* {
return self.strings.save(s).data();
};
toolchain::QueryParams params = {file, directory, query_args, callback};
auto result = toolchain::query_toolchain(params);
+ // Don't cache empty results to allow retries on transient failures.
+ if(result.empty()) {
+ return {};
+ }
+
auto [entry, _] = self.toolchain_cache.try_emplace(std::move(key), std::move(result));
return entry->second;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto callback = [&](const char* s) -> const char* { | |
| return self.strings.save(s).data(); | |
| }; | |
| toolchain::QueryParams params = {file, directory, query_args, callback}; | |
| auto result = toolchain::query_toolchain(params); | |
| auto [entry, _] = self.toolchain_cache.try_emplace(std::move(key), std::move(result)); | |
| return entry->second; | |
| } | |
| auto callback = [&](const char* s) -> const char* { | |
| return self.strings.save(s).data(); | |
| }; | |
| toolchain::QueryParams params = {file, directory, query_args, callback}; | |
| auto result = toolchain::query_toolchain(params); | |
| // Don't cache empty results to allow retries on transient failures. | |
| if(result.empty()) { | |
| return {}; | |
| } | |
| auto [entry, _] = self.toolchain_cache.try_emplace(std::move(key), std::move(result)); | |
| return entry->second; | |
| } |
🤖 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 137 - 145, The code
currently caches whatever toolchain::query_toolchain(params) returns into
self.toolchain_cache via try_emplace(key, result), which persists empty vectors
and prevents retries; change the logic in the function that builds params and
calls query_toolchain so that after obtaining auto result you check if result is
empty and only insert into self.toolchain_cache when non-empty (i.e., skip
try_emplace when result.empty()), returning the fresh result directly in the
empty case so callers can retry later; reference symbols:
toolchain::query_toolchain, toolchain::QueryParams, result, key,
self.toolchain_cache, try_emplace.
Comment on lines
+14
to
+36
| struct PathPool { | ||
| llvm::BumpPtrAllocator allocator; | ||
| llvm::SmallVector<llvm::StringRef> paths; | ||
| llvm::StringMap<std::uint32_t> cache; | ||
|
|
||
| std::uint32_t intern(llvm::StringRef path) { | ||
| auto [it, inserted] = cache.try_emplace(path, paths.size()); | ||
| if(inserted) { | ||
| // Allocate with null terminator so that resolve().data() is safe | ||
| // to use as const char* (e.g. in MemoryBuffer::getFile which calls strlen). | ||
| const std::size_t n = path.size(); | ||
| char* buf = allocator.Allocate<char>(n + 1); | ||
| std::copy(path.begin(), path.end(), buf); | ||
| buf[n] = '\0'; | ||
| paths.push_back(llvm::StringRef(buf, n)); | ||
| } | ||
| return it->second; | ||
| } | ||
|
|
||
| llvm::StringRef resolve(std::uint32_t id) const { | ||
| return paths[id]; | ||
| } | ||
| }; |
There was a problem hiding this comment.
ID 0 should be reserved as a sentinel value for consistency with StringSet.
The existing StringSet in src/support/object_pool.h reserves ID 0 as a sentinel for "no string" / "unset" (see Lines 23, 33-34). PathPool assigns 0 as the first valid ID, which breaks this convention. Code that checks path_id == 0 to detect "unset" will incorrectly match the first interned path.
Additionally, resolve() lacks bounds checking and will crash on invalid IDs.
🛠️ Proposed fix to reserve ID 0 and add bounds check
struct PathPool {
llvm::BumpPtrAllocator allocator;
llvm::SmallVector<llvm::StringRef> paths;
llvm::StringMap<std::uint32_t> cache;
+ PathPool() {
+ paths.emplace_back(); // Reserve index 0 as sentinel (empty path)
+ }
+
std::uint32_t intern(llvm::StringRef path) {
+ if(path.empty()) {
+ return 0; // Sentinel for empty/unset
+ }
auto [it, inserted] = cache.try_emplace(path, paths.size());
if(inserted) {
// Allocate with null terminator so that resolve().data() is safe
// to use as const char* (e.g. in MemoryBuffer::getFile which calls strlen).
const std::size_t n = path.size();
char* buf = allocator.Allocate<char>(n + 1);
std::copy(path.begin(), path.end(), buf);
buf[n] = '\0';
paths.push_back(llvm::StringRef(buf, n));
}
return it->second;
}
llvm::StringRef resolve(std::uint32_t id) const {
+ assert(id < paths.size() && "Invalid path ID");
return paths[id];
}
};🤖 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 - 36, PathPool currently hands out 0
as a valid ID and has no bounds checking in resolve; change PathPool so ID 0 is
reserved as a sentinel by inserting an initial empty entry into paths (and
ensure cache indices start from 1) before any intern() calls, adjust
intern(llvm::StringRef path) to assign IDs starting at 1 (e.g., use paths.size()
after the sentinel) and keep existing null-terminated allocation logic, and
update resolve(std::uint32_t id) to check that id != 0 and id < paths.size()
returning an empty llvm::StringRef (or assert/handle error) for invalid ids to
avoid crashes.
- Move test files to tests/unit/command/ matching source relocation - Add SearchConfig tests (8) and ToolchainProvider tests (11) - Fix cache key to include -x language mode and CommandOptions - Fix Quoted dedup bug (don't seed seen set with Quoted paths) - Remove CommandOptions::resource_dir, make it part of query_toolchain - Move resource_dir from global init to CompilationDatabase::resource_dir() using Driver::GetResourcesPath + getMainExecutable (no argv[0] dependency) - Skip flaky InlayHint.Special test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/unit/feature/inlay_hint_tests.cpp (1)
1339-1339: Consider tracking this skipped test with an issue or TODO comment.The
Specialtest case is being skipped, which the PR notes as a "pre-existing failure." While skipping is reasonable to unblock the PR, consider adding a TODO comment or linking to an issue to ensure this doesn't remain permanently skipped and forgotten.💡 Suggested improvement
-TEST_CASE(Special, {.skip = true}) { +// TODO(`#XXX`): Re-enable once resource-dir initialization is stabilized +TEST_CASE(Special, {.skip = true}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/feature/inlay_hint_tests.cpp` at line 1339, Add a TODO or issue link next to the skipped test declaration TEST_CASE(Special, {.skip = true}) so the skip is tracked; update the test file to include a short comment like "TODO: track pre-existing failure - see ISSUE-XXXX" (or a URL) and optionally include a brief reason and expected behavior, ensuring the comment sits immediately above or inline with the TEST_CASE(Special, {.skip = true}) declaration so reviewers can find and resolve it later.tests/unit/command/toolchain_provider_tests.cpp (1)
28-45: Test assertion does not verify the claimed behavior.The comment on line 42 states "query_cached with same key should return the first result," but the test only verifies
has_cached_entries()returns true. This doesn't confirm whether the first or second result was retained.Consider adding a verification that actually queries the cached value and checks which triple was stored, or remove the misleading comment.
🤖 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's comment claims "query_cached with same key should return the first result" but only checks has_cached_entries(); update TEST_CASE InjectResultsSkipsDuplicateKeys to actually fetch the cached entry for "key1" (use ToolchainProvider::query_cached or the provider method that returns the cached ToolchainResult) and assert that the stored triple/arguments equal the first injected result ("x86_64"), or if you cannot query the cache from the test, remove or update the misleading comment to reflect what the test actually verifies (presence of cached entries via has_cached_entries()).tests/unit/command/search_config_tests.cpp (1)
133-161: Clarify prefix concatenation semantics in test.The test relies on
tmp.path("gcc/12/")returning a path with a trailing separator (line 137), but this behavior may be platform-specific or implementation-dependent. The comment on line 136 mentions this requirement, which is good documentation.However, consider making the test more robust by explicitly constructing paths with the separator, or adding an assertion that
prefix12ends with a path separator to catch any futureTempDir::path()behavior changes.🤖 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 133 - 161, In TEST_CASE PrefixIncludeOptions, make the prefix concatenation explicit and robust by ensuring prefix12 and prefix13 include a trailing path separator before using them (or assert TempDir::path() returns one); modify the setup around TempDir tmp and the variables prefix12/prefix13 so you either append the platform-specific separator (use e.g. std::filesystem::path::preferred_separator) or add an assertion that prefix12.back() is a separator, so extract_search_config and the EXPECT_EQ checks that compare config.dirs[*].path against tmp.path("gcc/12/include") remain correct and platform-independent.
🤖 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 161-163: The cache key built by options_bits currently encodes
only query_toolchain, but lookup() (and thus lookup_search_config) also mutates
results based on CommandOptions::append and CommandOptions::remove, so different
append/remove values can return stale SearchConfig; update options_bits(const
CommandOptions&) to incorporate append/remove into the key (e.g., include
presence or a simple hash of the append/remove vectors/strings) so that
lookup()/lookup_search_config and cached SearchConfig entries are differentiated
by those values, or alternatively document in the lookup_search_config API that
append/remove must be constant for a given CompilationInfo; touch the functions
options_bits, CommandOptions, lookup(), and lookup_search_config when applying
this change.
- Around line 170-193: is_same_file currently normalizes slashes on Windows but
still does case-sensitive compares causing paths like "C:\Foo\Bar.cpp" vs
"c:\foo\bar.cpp" to be considered different; update the Windows-only branch in
is_same_file to perform a case-insensitive comparison (e.g., convert characters
to a common case or use a locale-independent tolower on each character) when
comparing normalized characters (argument and file) so that both slash and case
differences are ignored; ensure you apply the case normalization in the same
per-character loop (or normalize whole strings before comparing) and use safe
casts for char-to-unsigned-char when calling tolower to avoid UB.
- Around line 773-776: The unconditional arguments.pop_back() after
arguments.assign(cached.begin(), cached.end()) is unsafe; before removing the
last element, check that arguments.size() > 0 (or >1 if you require at least one
retained argument) and that the last element is actually the temporary source
filename (e.g., matches the temp file pattern or a known marker). If the checks
fail, skip pop_back() (or log a warning) to avoid removing a required flag;
update the block surrounding arguments, cached, and pop_back() accordingly.
In `@tests/unit/command/toolchain_provider_tests.cpp`:
- Around line 47-74: The test GetPendingQueriesReturnsUncachedOnly injects a
cached result with key "preloaded_key" but constructs PendingEntry instances
that compute a different toolchain key, so the test only exercises deduplication
between entry1 and entry2 rather than the "skip cached" behavior; update the
test by injecting a result whose key matches the computed key for the
constructed entries (use ToolchainProvider::inject_results with the actual
computed key pattern used by ToolchainProvider's get_pending_queries logic) or
else rename the test to reflect it only verifies duplicate-entry deduplication;
locate symbols ToolchainProvider, inject_results, get_pending_queries, and
PendingEntry to make the change.
---
Nitpick comments:
In `@tests/unit/command/search_config_tests.cpp`:
- Around line 133-161: In TEST_CASE PrefixIncludeOptions, make the prefix
concatenation explicit and robust by ensuring prefix12 and prefix13 include a
trailing path separator before using them (or assert TempDir::path() returns
one); modify the setup around TempDir tmp and the variables prefix12/prefix13 so
you either append the platform-specific separator (use e.g.
std::filesystem::path::preferred_separator) or add an assertion that
prefix12.back() is a separator, so extract_search_config and the EXPECT_EQ
checks that compare config.dirs[*].path against tmp.path("gcc/12/include")
remain correct and platform-independent.
In `@tests/unit/command/toolchain_provider_tests.cpp`:
- Around line 28-45: The test's comment claims "query_cached with same key
should return the first result" but only checks has_cached_entries(); update
TEST_CASE InjectResultsSkipsDuplicateKeys to actually fetch the cached entry for
"key1" (use ToolchainProvider::query_cached or the provider method that returns
the cached ToolchainResult) and assert that the stored triple/arguments equal
the first injected result ("x86_64"), or if you cannot query the cache from the
test, remove or update the misleading comment to reflect what the test actually
verifies (presence of cached entries via has_cached_entries()).
In `@tests/unit/feature/inlay_hint_tests.cpp`:
- Line 1339: Add a TODO or issue link next to the skipped test declaration
TEST_CASE(Special, {.skip = true}) so the skip is tracked; update the test file
to include a short comment like "TODO: track pre-existing failure - see
ISSUE-XXXX" (or a URL) and optionally include a brief reason and expected
behavior, ensuring the comment sits immediately above or inline with the
TEST_CASE(Special, {.skip = true}) declaration so reviewers can find and resolve
it later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b650d91c-26bd-4957-ab0e-3cd0b711af58
📒 Files selected for processing (16)
src/clice.ccsrc/command/command.cppsrc/command/command.hsrc/command/search_config.cppsrc/command/toolchain_provider.cppsrc/server/master_server.cppsrc/support/filesystem.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/tester.cpptests/unit/unit_tests.cc
💤 Files with no reviewable changes (3)
- tests/unit/unit_tests.cc
- tests/unit/test/tester.cpp
- src/support/filesystem.h
Comment on lines
+161
to
+163
| static std::uint8_t options_bits(const CommandOptions& options) { | ||
| return options.query_toolchain ? 1u : 0u; | ||
| } |
There was a problem hiding this comment.
options_bits does not encode append/remove options that affect lookup() output.
The options_bits() function only encodes query_toolchain, but lookup() also uses options.append and options.remove (lines 487-569) to modify the final arguments. If the same CompilationInfo is looked up with different append/remove options, the cache will return a stale SearchConfig.
Consider either:
- Including a hash of
append/removein the cache key, or - Documenting that
lookup_search_configmust only be called with consistentappend/removefor a given context.
Option 1: Extend options_bits to include append/remove presence
static std::uint8_t options_bits(const CommandOptions& options) {
- return options.query_toolchain ? 1u : 0u;
+ std::uint8_t bits = 0;
+ if(options.query_toolchain) bits |= 1u;
+ if(!options.append.empty()) bits |= 2u;
+ if(!options.remove.empty()) bits |= 4u;
+ return bits;
}Note: This only distinguishes empty vs non-empty; a full solution would hash the actual content.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static std::uint8_t options_bits(const CommandOptions& options) { | |
| return options.query_toolchain ? 1u : 0u; | |
| } | |
| static std::uint8_t options_bits(const CommandOptions& options) { | |
| std::uint8_t bits = 0; | |
| if(options.query_toolchain) bits |= 1u; | |
| if(!options.append.empty()) bits |= 2u; | |
| if(!options.remove.empty()) bits |= 4u; | |
| return bits; | |
| } |
🤖 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 cache key built by
options_bits currently encodes only query_toolchain, but lookup() (and thus
lookup_search_config) also mutates results based on CommandOptions::append and
CommandOptions::remove, so different append/remove values can return stale
SearchConfig; update options_bits(const CommandOptions&) to incorporate
append/remove into the key (e.g., include presence or a simple hash of the
append/remove vectors/strings) so that lookup()/lookup_search_config and cached
SearchConfig entries are differentiated by those values, or alternatively
document in the lookup_search_config API that append/remove must be constant for
a given CompilationInfo; touch the functions options_bits, CommandOptions,
lookup(), and lookup_search_config when applying this change.
Comment on lines
+170
to
+193
| /// Check if an argument matches the source file path, handling | ||
| /// Windows path separator differences (backslash vs forward slash). | ||
| static bool is_same_file(llvm::StringRef argument, llvm::StringRef file) { | ||
| if(argument == file) { | ||
| return true; | ||
| } | ||
|
|
||
| #ifdef _WIN32 | ||
| // On Windows, cmake may use backslashes in `arguments` but forward | ||
| // slashes in `file`. Normalize and compare. | ||
| if(argument.size() == file.size()) { | ||
| for(std::size_t i = 0; i < argument.size(); i++) { | ||
| char a = argument[i] == '\\' ? '/' : argument[i]; | ||
| char b = file[i] == '\\' ? '/' : file[i]; | ||
| if(a != b) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| #endif | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Windows path comparison may fail on case differences.
The is_same_file() function normalizes slash direction but doesn't handle case-insensitivity. On Windows, C:\Foo\Bar.cpp and c:\foo\bar.cpp refer to the same file, but this function would return false.
If CMake or build systems generate paths with inconsistent casing, the source file won't be filtered from arguments (line 208), potentially causing duplicate entries or unexpected behavior.
Suggested case-insensitive comparison for Windows
`#ifdef` _WIN32
// On Windows, cmake may use backslashes in `arguments` but forward
// slashes in `file`. Normalize and compare.
if(argument.size() == file.size()) {
for(std::size_t i = 0; i < argument.size(); i++) {
char a = argument[i] == '\\' ? '/' : argument[i];
char b = file[i] == '\\' ? '/' : file[i];
+ // Case-insensitive comparison on Windows
+ a = std::tolower(static_cast<unsigned char>(a));
+ b = std::tolower(static_cast<unsigned char>(b));
if(a != b) {
return false;
}
}
return true;
}
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/command.cpp` around lines 170 - 193, is_same_file currently
normalizes slashes on Windows but still does case-sensitive compares causing
paths like "C:\Foo\Bar.cpp" vs "c:\foo\bar.cpp" to be considered different;
update the Windows-only branch in is_same_file to perform a case-insensitive
comparison (e.g., convert characters to a common case or use a
locale-independent tolower on each character) when comparing normalized
characters (argument and file) so that both slash and case differences are
ignored; ensure you apply the case normalization in the same per-character loop
(or normalize whole strings before comparing) and use safe casts for
char-to-unsigned-char when calling tolower to avoid UB.
Comment on lines
+773
to
+776
| arguments.assign(cached.begin(), cached.end()); | ||
|
|
||
| // Remove the temp source file that was appended during query. | ||
| arguments.pop_back(); |
There was a problem hiding this comment.
Unconditional pop_back() assumes cached result always has a trailing temp source file.
Line 776 calls arguments.pop_back() without checking if arguments has more than one element. While the code path only executes when cached is non-empty (checked on line 768), if the cached result somehow contains only the -cc1 flag or minimal elements, this could remove a required argument.
Consider adding a size check for safety:
Suggested defensive check
// Start with cc1 result (has system paths, driver flags, etc.).
arguments.assign(cached.begin(), cached.end());
- // Remove the temp source file that was appended during query.
- arguments.pop_back();
+ // Remove the temp source file that was appended during query.
+ if(!arguments.empty()) {
+ arguments.pop_back();
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/command.cpp` around lines 773 - 776, The unconditional
arguments.pop_back() after arguments.assign(cached.begin(), cached.end()) is
unsafe; before removing the last element, check that arguments.size() > 0 (or >1
if you require at least one retained argument) and that the last element is
actually the temporary source filename (e.g., matches the temp file pattern or a
known marker). If the checks fail, skip pop_back() (or log a warning) to avoid
removing a required flag; update the block surrounding arguments, cached, and
pop_back() accordingly.
Comment on lines
+47
to
+74
| TEST_CASE(GetPendingQueriesReturnsUncachedOnly) { | ||
| ToolchainProvider provider; | ||
|
|
||
| // Inject a result for one toolchain configuration. | ||
| std::vector<ToolchainResult> results; | ||
| results.push_back({ | ||
| "preloaded_key", | ||
| {"-cc1", "-triple", "x86_64"} | ||
| }); | ||
| provider.inject_results(results); | ||
|
|
||
| // Create entries: one matching the cached key pattern, one new. | ||
| // Since get_pending_queries extracts keys internally, we need real-ish args. | ||
| ToolchainProvider::PendingEntry entry1; | ||
| entry1.file = "a.cpp"; | ||
| entry1.directory = "/tmp"; | ||
| entry1.arguments = {"clang++", "-std=c++17", "a.cpp"}; | ||
|
|
||
| ToolchainProvider::PendingEntry entry2; | ||
| entry2.file = "b.cpp"; | ||
| entry2.directory = "/tmp"; | ||
| entry2.arguments = {"clang++", "-std=c++17", "b.cpp"}; | ||
|
|
||
| // Both entries have the same toolchain key (same driver, same extension, | ||
| // same toolchain flags), so only one query should be returned. | ||
| auto queries = provider.get_pending_queries({entry1, entry2}); | ||
| EXPECT_EQ(queries.size(), 1u); | ||
| } |
There was a problem hiding this comment.
Test may not verify intended behavior due to key mismatch.
The test injects a result with key "preloaded_key" (line 53), but the entries constructed on lines 60-68 will generate a completely different computed key based on clang++, file extension, and toolchain flags. The comment on line 58 says "one matching the cached key pattern" but no entry actually matches "preloaded_key".
As a result, this test only verifies deduplication of identical keys between entry1 and entry2, not the "returns uncached only" behavior claimed in the test name.
Consider either:
- Inject a result using an actual computed key (as done in
InjectThenGetPendingSkipsCached), or - Rename/refactor the test to accurately reflect what it verifies.
🤖 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 GetPendingQueriesReturnsUncachedOnly injects a cached result with key
"preloaded_key" but constructs PendingEntry instances that compute a different
toolchain key, so the test only exercises deduplication between entry1 and
entry2 rather than the "skip cached" behavior; update the test by injecting a
result whose key matches the computed key for the constructed entries (use
ToolchainProvider::inject_results with the actual computed key pattern used by
ToolchainProvider's get_pending_queries logic) or else rename the test to
reflect it only verifies duplicate-entry deduplication; locate symbols
ToolchainProvider, inject_results, get_pending_queries, and PendingEntry to make
the change.
Member
Author
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
SearchConfigextraction pipeline modeling clang's four-segment include directory layout (Quoted → Angled → System → After)ToolchainProvidercache forclang++ -###query results, keyed by toolchain-relevant flagsPathPoolintern table for lightweight file-path identity (uint32_t IDs)TempDirRAII test helper with cross-platform path handlingcommand.h/cppwithlookup_search_config,resolve_toolchain_entries,toolchain(),has_cached_configs,resolve_pathAPIsExtractSearchConfigunit testsTest plan
InlayHint.Specialfailure)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests