feat: add include resolver, dependency graph, BFS scanner - #368
Merged
Merged
Conversation
16bit-ykiko
force-pushed
the
feat/search-config
branch
from
March 25, 2026 15:57
b161085 to
73bc73b
Compare
16bit-ykiko
force-pushed
the
feat/dependency-graph
branch
from
March 25, 2026 15:57
91d9b27 to
5ad5af7
Compare
|
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 wavefront BFS include dependency scanner and cache, include-resolution utilities, PathPool-based path interning and CompilationDatabase accessors, a benchmark CLI and CI workflow, server integration to run scans during workspace load, CMake/bench target, and extensive unit tests for scanning and resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant API as scan_dependency_graph
participant CDB as CompilationDatabase
participant PathPool as PathPool
participant Cache as ScanCache / DirListingCache
participant Phase1 as Phase 1: Read & Lex
participant Phase2 as Phase 2: Resolve & Build
participant Graph as DependencyGraph
Caller->>API: scan_dependency_graph(cdb, path_pool, graph, cache?)
API->>CDB: read entries, group contexts
API->>Cache: prewarm dir listings & context caches
API->>Phase1: launch parallel reads & lex (use cache if available)
Phase1->>Cache: produce/check ScanResult
Phase1-->>API: return scan results for wave
API->>Phase2: resolve includes using ResolvedSearchConfig
Phase2->>Cache: resolve_include(...) using DirListingCache
Phase2->>PathPool: intern resolved paths
Phase2->>Graph: add edges (mark conditional/unconditional), register modules
API->>API: assemble next wave until no work
API-->>Caller: return ScanReport (timings, counts, unresolved)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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 (18)
src/support/path_pool.h (1)
33-35: Consider adding bounds check or assertion inresolve().If an invalid
idis passed (e.g., from stale data or corruption), this will access out-of-bounds memory. Consider adding an assertion for debug builds: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 33 - 35, The resolve(std::uint32_t id) method in path_pool.h currently indexes paths[id] without validation; add a bounds check or debug assertion (e.g., assert(id < paths.size() && "Invalid path ID")) before returning to prevent out-of-bounds access, or alternatively handle the error path (throw or return an empty/optional llvm::StringRef) so resolve and the paths container are protected from stale/corrupt IDs..github/workflows/benchmark.yml (2)
32-42: Consider caching LLVM clone to speed up CI.Cloning
llvm-project(even with--depth 1) and running CMake configure on it is time-consuming. Consider caching the clone between runs:♻️ Example caching approach
- name: Cache LLVM clone uses: actions/cache@v4 with: path: llvm-project key: llvm-project-${{ runner.os }}-depth1 - name: Clone LLVM if: steps.cache-llvm.outputs.cache-hit != 'true' run: git clone --depth 1 https://github.com/llvm/llvm-project.git🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/benchmark.yml around lines 32 - 42, Add a cache step before the existing "Clone LLVM" step to persist the llvm-project directory between runs using actions/cache@v4, then make the "Clone LLVM" step conditional on the cache miss by checking the cache step's cache-hit output; specifically, insert a "Cache LLVM clone" step that caches path llvm-project with a key like llvm-project-${{ runner.os }}-depth1 and change the "Clone LLVM" step to run only when that cache step reports cache-hit != 'true' so the "Generate CDB" step (the cmake invocation) can reuse the cached clone when available.
46-47: Benchmark results are not persisted.The benchmark output only appears in CI logs. Consider uploading results as artifacts or using a benchmarking action (e.g.,
benchmark-action/github-action-benchmark) to track performance over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/benchmark.yml around lines 46 - 47, The "Run benchmark" step that executes "./build/RelWithDebInfo/bin/scan_benchmark --runs 20 llvm-build/compile_commands.json" currently only prints results to CI logs; modify the workflow to persist results by saving the benchmark output to a file (e.g., redirect stdout/stderr to a results file) and then upload that file as a GitHub Actions artifact (add an "actions/upload-artifact" step referencing the results filename), or replace/enhance the step with a benchmarking action such as benchmark-action/github-action-benchmark to automatically record and track metrics over time.src/syntax/scan.cpp (1)
7-7: Unused include:llvm/ADT/StringSet.hThis header does not appear to be used anywhere in this file. No
StringSettype is referenced.🧹 Remove unused include
-#include "llvm/ADT/StringSet.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/scan.cpp` at line 7, Remove the unused header include "llvm/ADT/StringSet.h" from src/syntax/scan.cpp — the file does not reference the StringSet type anywhere; delete the line `#include` "llvm/ADT/StringSet.h" and run a quick rebuild to ensure no missing symbols remain.tests/unit/test/temp_dir.h (2)
26-28: Unchecked error fromcreateUniqueDirectory.If directory creation fails,
rootwill be empty, and subsequent operations will silently fail or operate on unexpected paths. Consider at least asserting success in test code:TempDir(llvm::StringRef prefix = "clice-test") { auto ec = llvm::sys::fs::createUniqueDirectory(prefix, root); assert(!ec && "Failed to create temp directory"); }🤖 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 calls llvm::sys::fs::createUniqueDirectory(prefix, root) but ignores the returned error code so root may be empty on failure; modify the TempDir(llvm::StringRef prefix) constructor to capture the returned llvm::ErrorCode (or std::error_code) from createUniqueDirectory, assert or handle failure (e.g., assert(!ec, "Failed to create temp directory") or log and abort) and ensure root is only used when the call succeeds; reference TempDir, createUniqueDirectory, and root when making the change.
59-67: Silent failure intouch()can hide test setup issues.If file creation or writing fails, the test will proceed with missing/empty files, potentially causing confusing test failures. Consider asserting success:
🛠️ Proposed fix
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"); + 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 ignores filesystem errors and may silently produce missing/empty files; update touch (and its use of path(), llvm::sys::fs::create_directories and llvm::raw_fd_ostream) to detect and fail on errors: check the result of create_directories and the std::error_code ec from llvm::raw_fd_ostream, and if an error is present, fail the test (e.g., with an assertion or throwing a test-fatal exception) including the path and ec.message(); also ensure the output is properly flushed/closed after writing.tests/unit/syntax/dependency_graph_tests.cpp (1)
373-385: Direct access topool.cachefor path lookup.Line 375 accesses
pool.cache[tmp.path("src/main.cpp")]to get the path ID. This assumesPathPool::cacheis public and uses a map-like interface. If this is test-only access, consider adding aPathPool::lookup(path)method that returnsstd::optional<uint32_t>for cleaner test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/dependency_graph_tests.cpp` around lines 373 - 385, The test directly reads pool.cache[tmp.path("src/main.cpp")] which relies on PathPool::cache being public; change the test to use a new PathPool lookup API instead: add a PathPool::lookup(const std::string& path) returning std::optional<uint32_t> (or use existing pool.lookup if present), call that with tmp.path("src/main.cpp"), check the optional is set, then pass the contained id to graph.get_includes; update the test to assert the lookup succeeded before using the id and remove direct accesses to pool.cache.benchmarks/scan_benchmark.cpp (2)
74-94: Direct access topath_pool.pathscouples benchmark to internal structure.The export function directly accesses
path_pool.paths[id]andpath_pool.paths.size(). IfPathPool's internal representation changes, this will break.Consider adding a public iteration API to
PathPoolif this pattern is needed elsewhere, or document that this benchmark has privileged access to internals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 74 - 94, This loop in the benchmark directly reads PathPool internals (path_pool.paths and its size), coupling the benchmark to PathPool's representation; change the code to use a public PathPool API (e.g., add and call methods like PathPool::size(), PathPool::get_path(id) or an iterator begin()/end()) instead of accessing path_pool.paths, update the loop to iterate via that API when building FileNode (node.path and the includes pushed from path_pool.paths[raw_id].str()), and keep use of graph.get_all_includes, DependencyGraph::PATH_ID_MASK, path_to_module, FileNode and export_data.files intact so callers only touch PathPool's public interface.
254-258:putenvusage is correct but considersetenvfor clarity.The
static std::string envensures the string persists for the program lifetime as required byputenv. However,setenv(POSIX) is generally preferred as it copies the string internally.Note:
setenvis not available on Windows, so if cross-platform support is needed, thisputenvpattern with a static string is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 254 - 258, Replace the putenv usage with setenv when available to let the runtime copy the string: check for UV_THREADPOOL_SIZE via std::getenv as shown, compute pool_size, then call setenv("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str(), 1) on POSIX; retain the existing static std::string + putenv(env.data()) fallback for Windows or when setenv is unavailable (use `#ifdef` _WIN32 or HAVE_SETENV guard). Update the code around getenv/UV_THREADPOOL_SIZE/pool_size/env/putenv to prefer setenv but fall back to the current putenv pattern for cross-platform compatibility.src/command/search_config.cpp (1)
59-67: Prefix path concatenation assumes prefix includes trailing separator.When
-iprefix /usr/include(no trailing/) is followed by-iwithprefix lib, the result is/usr/includelibinstead of/usr/include/lib. This matches clang's behavior (which also directly concatenates), but it may be surprising.The test at line 432-433 in
command_tests.cppusestmp.path("gcc/12/")with a trailing slash, which works correctly. Consider adding a test case without trailing slash to document this behavior.🤖 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 prefix concatenation code (cases for ID::OPT_iprefix and ID::OPT_iwithprefix using make_absolute) assumes the prefix already has a trailing separator and will produce "/usr/includelib" when given "-iprefix /usr/include" + "-iwithprefix lib"; add a unit test in the command_tests suite that constructs a prefix without a trailing slash (e.g., tmp.path("gcc/12") or "/usr/include") and then invokes the code path that parses OPT_iprefix + OPT_iwithprefix, asserting the produced path equals a direct concatenation of prefix + arg (i.e., no added slash) so the behavior is documented; update or add an assertion near the existing test that currently uses tmp.path("gcc/12/") to include a sibling case without the trailing "/" to capture this behavior.src/command/toolchain_provider.cpp (1)
29-44: Consider adding-cxx-isystemand potentially-iframeworkto toolchain options if supporting these paths becomes necessary.The
is_toolchain_optionfilter captures key system path discovery flags. However,-cxx-isystem(C++-specific system directories) and-iframework(macOS frameworks) can affect system includes and are missing from the filter. A TODO comment insearch_config.cppalready flags-cxx-isystemas a known gap, suggesting awareness of this limitation. These options are not currently used in the codebase, so this remains a low-priority enhancement for completeness rather than a bug fix.🤖 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 29 - 44, The is_toolchain_option filter (function is_toolchain_option) omits C++-specific and framework include flags; add cases for the corresponding enum values (e.g., ID::OPT_cxx_isystem and ID::OPT_iframework or whatever the project enum names are) to return true so these flags are treated as toolchain options like OPT_isysroot and OPT__sysroot_EQ; update the switch in is_toolchain_option to include those enum entries and run a quick compile to ensure the enum names match the codebase.src/command/toolchain_provider.h (1)
15-20: Document lifetime requirements for StringRef members.
ToolchainQuery::fileanddirectoryarellvm::StringRefwhich don't own their data. Callers must ensure the referenced strings outlive the query execution. Consider adding a doc comment or usingstd::stringif ownership is unclear.🤖 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 15 - 20, The ToolchainQuery struct's members file and directory are llvm::StringRef and do not own their data; update the code by documenting lifetime requirements on ToolchainQuery (add a comment above struct ToolchainQuery stating that callers must ensure the storage backing file and directory outlives the query execution) or, if ownership is unclear, change those members to std::string to take ownership; reference the ToolchainQuery struct and its file and directory members when making the change so callers and implementors see the lifetime requirement or new ownership semantics.src/syntax/dependency_graph.cpp (3)
71-83: First-seen flag wins inget_all_includes— consider if this is intended.When merging includes across configs, deduplication uses
raw_idbut stores the original flaggedid. If a header is included conditionally in config A and unconditionally in config B, only the first-encountered flag is preserved. If the intent is to track "ever unconditional," this logic should be adjusted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/dependency_graph.cpp` around lines 71 - 83, get_all_includes currently deduplicates by raw_id (using PATH_ID_MASK) but keeps the first-seen full id (including any conditional/unconditional flags), so an unconditional include seen after a conditional one won't upgrade the stored flag; change the merge logic in get_all_includes to detect when a later id has a "stronger" flag (e.g., unconditional) and replace the previously stored id in result (and update seen bookkeeping) instead of keeping the first one; locate the loop over fc_it->second and the use of seen, result, IncludeKey and PATH_ID_MASK and implement a compare/replace step that preserves the strongest include flag for each raw_id.
631-643: Phase 3 timing is always zero — remove dead code.
phase3_endis assigned the same value asphase2_end(line 632), sop3is always 0. The comment at line 478 confirms Phase 2 and 3 were merged. Consider removingphase3_end,p3, andreport.phase3_msto reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/dependency_graph.cpp` around lines 631 - 643, The code calculates a redundant phase3 timing: phase3_end is set equal to phase2_end so p3 is always zero; remove the dead variables and update the report accumulation to reflect only two phases. Specifically, delete phase3_end, the computation of p3, and the accumulation report.phase3_ms += p3; keep the existing computations for p1 and p2 that use wave_start and phase1_end/phase2_end and only add p1 to report.phase1_ms and p2 to report.phase2_ms (symbols to edit: phase2_end, phase3_end, p1, p2, p3, report.phase3_ms, report.phase2_ms, wave_start, phase1_end).
334-352: Consider avoiding copy of initial_wave on warm runs.Line 336 copies the entire
initial_wavevector. For large codebases this adds overhead. If the cached wave is only consumed (not reused across multiple scans), a move or reference could eliminate the copy.♻️ Potential optimization
const bool have_initial_wave_cache = ext_cache && !ext_cache->initial_wave.empty(); if(have_initial_wave_cache) { - current_wave = ext_cache->initial_wave; + current_wave = std::move(ext_cache->initial_wave); for(auto& entry: current_wave) { scanned_files.try_emplace(entry.path_id, entry.found_dir_idx); } + // Rebuild initial_wave at end if cache needs to persist across scans } else {Note: This requires restoring
initial_waveafter the scan if the cache should be reusable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/dependency_graph.cpp` around lines 334 - 352, The code currently copies ext_cache->initial_wave into current_wave causing unnecessary allocation; change the logic in the branch that checks have_initial_wave_cache to move the cached vector into current_wave (e.g., use std::move or swap with current_wave) and then populate scanned_files from the moved current_wave entries; if ext_cache->initial_wave must remain reusable afterwards, move it into a local temporary for the scan and then restore it (copy or move back) after the scan completes. Ensure you only modify the block that handles ext_cache/initial_wave and keep the rest of current_wave/scanned_files usage unchanged.src/syntax/dependency_graph.h (1)
134-141:phase3_msis always zero — consider removing or documenting.The implementation merged Phase 2 and 3, making
phase3_msalways zero. Either remove this field or update the comment to indicate it's reserved for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/dependency_graph.h` around lines 134 - 141, The field phase3_ms is always zero because Phase 2 and 3 were merged; either remove the phase3_ms member declaration to avoid dead state or explicitly mark it as reserved: update its comment to "reserved for future use (always zero — phases merged)" and keep the field for ABI/serialization compatibility; also search for any uses/serialization of phase3_ms and remove or adapt them accordingly.src/command/command.cpp (1)
907-949: Consider extracting context lookup helper to reduce duplication.The pattern of looking up
CompilationInfo*from(path_id, context)appears inlookup(),lookup_search_config(), andresolve_toolchain_entries(). Extracting a helper likefind_compilation_info(path_id, context)would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/command.cpp` around lines 907 - 949, The code duplicates the logic that finds a CompilationInfo for a given path_id and context in lookup(), lookup_search_config(), and resolve_toolchain_entries(); extract that into a helper like find_compilation_info(path_id, context) that traverses self->files[path_id] and its linked list (checking cur->info.ptr == context) and returns object_ptr<CompilationInfo> (or nullptr). Replace the duplicated blocks in resolve_toolchain_entries (and similarly update lookup() and lookup_search_config()) to call find_compilation_info(path_id, context), then use the returned info for the arguments/directory logic.src/syntax/include_resolver.cpp (1)
118-123: Normalization detection may miss edge cases.The check for
"\\."(backslash-dot) only catches.\patterns but not standalone.as a path component without a following slash (e.g.,foo\.barwouldn't need normalization butfoo\.\barwould). However, this is a minor edge case unlikely to occur in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/include_resolver.cpp` around lines 118 - 123, The normalization detection in the needs_normalize computation misses cases where a dot component follows a path separator (e.g., "\." or "/.") or is a standalone component; update the boolean expression that computes needs_normalize (using filename and is_simple) to detect any path-separator + "."/ ".." sequences instead of just "\\.", for example by checking for "/." or "\\." or "/.." or "\\.." or testing for filename == "."/ ".." or using a small loop to scan for a path separator followed by '.' (so dot components like "\." and "\.." are correctly flagged for normalization).
🤖 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 772-776: The code currently assumes the last element of cached
(copied into arguments) is the temp source file appended during query_toolchain
and unconditionally calls arguments.pop_back(); instead of blindly popping,
validate the invariant: check the last argument from cached (or
arguments.back()) matches the expected temp-source pattern (e.g., filename
suffix/prefix or a sentinel token created by query_toolchain) before removing
it, and if it does not match, log or assert (or avoid removal) to avoid
discarding a legitimate compiler flag; also add a brief comment documenting that
query_toolchain appends a temp source and why the check is necessary.
In `@src/server/master_server.cpp`:
- Around line 197-199: scan_dependency_graph blocks the main event loop (it
calls loop.run()), causing load_workspace (which is scheduled on the main loop)
to block; fix by making the call non-blocking: either run scan_dependency_graph
on a background thread/worker (e.g., std::async/std::thread or a thread-pool
task) and then post the completed dependency_graph back to the main loop, or
refactor scan_dependency_graph into a coroutine-friendly/yielding version that
does not call loop.run() internally; ensure you update the call site in
load_workspace to await or receive the result asynchronously and protect shared
state (dependency_graph, cdb, updates, path_pool) for thread-safety.
In `@src/syntax/dependency_graph.cpp`:
- Around line 45-51: The method DependencyGraph::set_includes currently always
pushes config_id into file_configs[path_id], causing duplicate entries when
set_includes is called multiple times for the same (path_id, config_id); modify
set_includes to guard insertion by checking whether config_id is already present
in file_configs[path_id] (e.g., use std::find or equivalent on the SmallVector
before push_back) or otherwise deduplicate after insertion so
file_configs[path_id] contains each config_id only once while leaving the
includes[key] assignment as-is.
---
Nitpick comments:
In @.github/workflows/benchmark.yml:
- Around line 32-42: Add a cache step before the existing "Clone LLVM" step to
persist the llvm-project directory between runs using actions/cache@v4, then
make the "Clone LLVM" step conditional on the cache miss by checking the cache
step's cache-hit output; specifically, insert a "Cache LLVM clone" step that
caches path llvm-project with a key like llvm-project-${{ runner.os }}-depth1
and change the "Clone LLVM" step to run only when that cache step reports
cache-hit != 'true' so the "Generate CDB" step (the cmake invocation) can reuse
the cached clone when available.
- Around line 46-47: The "Run benchmark" step that executes
"./build/RelWithDebInfo/bin/scan_benchmark --runs 20
llvm-build/compile_commands.json" currently only prints results to CI logs;
modify the workflow to persist results by saving the benchmark output to a file
(e.g., redirect stdout/stderr to a results file) and then upload that file as a
GitHub Actions artifact (add an "actions/upload-artifact" step referencing the
results filename), or replace/enhance the step with a benchmarking action such
as benchmark-action/github-action-benchmark to automatically record and track
metrics over time.
In `@benchmarks/scan_benchmark.cpp`:
- Around line 74-94: This loop in the benchmark directly reads PathPool
internals (path_pool.paths and its size), coupling the benchmark to PathPool's
representation; change the code to use a public PathPool API (e.g., add and call
methods like PathPool::size(), PathPool::get_path(id) or an iterator
begin()/end()) instead of accessing path_pool.paths, update the loop to iterate
via that API when building FileNode (node.path and the includes pushed from
path_pool.paths[raw_id].str()), and keep use of graph.get_all_includes,
DependencyGraph::PATH_ID_MASK, path_to_module, FileNode and export_data.files
intact so callers only touch PathPool's public interface.
- Around line 254-258: Replace the putenv usage with setenv when available to
let the runtime copy the string: check for UV_THREADPOOL_SIZE via std::getenv as
shown, compute pool_size, then call setenv("UV_THREADPOOL_SIZE",
std::to_string(pool_size).c_str(), 1) on POSIX; retain the existing static
std::string + putenv(env.data()) fallback for Windows or when setenv is
unavailable (use `#ifdef` _WIN32 or HAVE_SETENV guard). Update the code around
getenv/UV_THREADPOOL_SIZE/pool_size/env/putenv to prefer setenv but fall back to
the current putenv pattern for cross-platform compatibility.
In `@src/command/command.cpp`:
- Around line 907-949: The code duplicates the logic that finds a
CompilationInfo for a given path_id and context in lookup(),
lookup_search_config(), and resolve_toolchain_entries(); extract that into a
helper like find_compilation_info(path_id, context) that traverses
self->files[path_id] and its linked list (checking cur->info.ptr == context) and
returns object_ptr<CompilationInfo> (or nullptr). Replace the duplicated blocks
in resolve_toolchain_entries (and similarly update lookup() and
lookup_search_config()) to call find_compilation_info(path_id, context), then
use the returned info for the arguments/directory logic.
In `@src/command/search_config.cpp`:
- Around line 59-67: The prefix concatenation code (cases for ID::OPT_iprefix
and ID::OPT_iwithprefix using make_absolute) assumes the prefix already has a
trailing separator and will produce "/usr/includelib" when given "-iprefix
/usr/include" + "-iwithprefix lib"; add a unit test in the command_tests suite
that constructs a prefix without a trailing slash (e.g., tmp.path("gcc/12") or
"/usr/include") and then invokes the code path that parses OPT_iprefix +
OPT_iwithprefix, asserting the produced path equals a direct concatenation of
prefix + arg (i.e., no added slash) so the behavior is documented; update or add
an assertion near the existing test that currently uses tmp.path("gcc/12/") to
include a sibling case without the trailing "/" to capture this behavior.
In `@src/command/toolchain_provider.cpp`:
- Around line 29-44: The is_toolchain_option filter (function
is_toolchain_option) omits C++-specific and framework include flags; add cases
for the corresponding enum values (e.g., ID::OPT_cxx_isystem and
ID::OPT_iframework or whatever the project enum names are) to return true so
these flags are treated as toolchain options like OPT_isysroot and
OPT__sysroot_EQ; update the switch in is_toolchain_option to include those enum
entries and run a quick compile to ensure the enum names match the codebase.
In `@src/command/toolchain_provider.h`:
- Around line 15-20: The ToolchainQuery struct's members file and directory are
llvm::StringRef and do not own their data; update the code by documenting
lifetime requirements on ToolchainQuery (add a comment above struct
ToolchainQuery stating that callers must ensure the storage backing file and
directory outlives the query execution) or, if ownership is unclear, change
those members to std::string to take ownership; reference the ToolchainQuery
struct and its file and directory members when making the change so callers and
implementors see the lifetime requirement or new ownership semantics.
In `@src/support/path_pool.h`:
- Around line 33-35: The resolve(std::uint32_t id) method in path_pool.h
currently indexes paths[id] without validation; add a bounds check or debug
assertion (e.g., assert(id < paths.size() && "Invalid path ID")) before
returning to prevent out-of-bounds access, or alternatively handle the error
path (throw or return an empty/optional llvm::StringRef) so resolve and the
paths container are protected from stale/corrupt IDs.
In `@src/syntax/dependency_graph.cpp`:
- Around line 71-83: get_all_includes currently deduplicates by raw_id (using
PATH_ID_MASK) but keeps the first-seen full id (including any
conditional/unconditional flags), so an unconditional include seen after a
conditional one won't upgrade the stored flag; change the merge logic in
get_all_includes to detect when a later id has a "stronger" flag (e.g.,
unconditional) and replace the previously stored id in result (and update seen
bookkeeping) instead of keeping the first one; locate the loop over
fc_it->second and the use of seen, result, IncludeKey and PATH_ID_MASK and
implement a compare/replace step that preserves the strongest include flag for
each raw_id.
- Around line 631-643: The code calculates a redundant phase3 timing: phase3_end
is set equal to phase2_end so p3 is always zero; remove the dead variables and
update the report accumulation to reflect only two phases. Specifically, delete
phase3_end, the computation of p3, and the accumulation report.phase3_ms += p3;
keep the existing computations for p1 and p2 that use wave_start and
phase1_end/phase2_end and only add p1 to report.phase1_ms and p2 to
report.phase2_ms (symbols to edit: phase2_end, phase3_end, p1, p2, p3,
report.phase3_ms, report.phase2_ms, wave_start, phase1_end).
- Around line 334-352: The code currently copies ext_cache->initial_wave into
current_wave causing unnecessary allocation; change the logic in the branch that
checks have_initial_wave_cache to move the cached vector into current_wave
(e.g., use std::move or swap with current_wave) and then populate scanned_files
from the moved current_wave entries; if ext_cache->initial_wave must remain
reusable afterwards, move it into a local temporary for the scan and then
restore it (copy or move back) after the scan completes. Ensure you only modify
the block that handles ext_cache/initial_wave and keep the rest of
current_wave/scanned_files usage unchanged.
In `@src/syntax/dependency_graph.h`:
- Around line 134-141: The field phase3_ms is always zero because Phase 2 and 3
were merged; either remove the phase3_ms member declaration to avoid dead state
or explicitly mark it as reserved: update its comment to "reserved for future
use (always zero — phases merged)" and keep the field for ABI/serialization
compatibility; also search for any uses/serialization of phase3_ms and remove or
adapt them accordingly.
In `@src/syntax/include_resolver.cpp`:
- Around line 118-123: The normalization detection in the needs_normalize
computation misses cases where a dot component follows a path separator (e.g.,
"\." or "/.") or is a standalone component; update the boolean expression that
computes needs_normalize (using filename and is_simple) to detect any
path-separator + "."/ ".." sequences instead of just "\\.", for example by
checking for "/." or "\\." or "/.." or "\\.." or testing for filename == "."/
".." or using a small loop to scan for a path separator followed by '.' (so dot
components like "\." and "\.." are correctly flagged for normalization).
In `@src/syntax/scan.cpp`:
- Line 7: Remove the unused header include "llvm/ADT/StringSet.h" from
src/syntax/scan.cpp — the file does not reference the StringSet type anywhere;
delete the line `#include` "llvm/ADT/StringSet.h" and run a quick rebuild to
ensure no missing symbols remain.
In `@tests/unit/syntax/dependency_graph_tests.cpp`:
- Around line 373-385: The test directly reads
pool.cache[tmp.path("src/main.cpp")] which relies on PathPool::cache being
public; change the test to use a new PathPool lookup API instead: add a
PathPool::lookup(const std::string& path) returning std::optional<uint32_t> (or
use existing pool.lookup if present), call that with tmp.path("src/main.cpp"),
check the optional is set, then pass the contained id to graph.get_includes;
update the test to assert the lookup succeeded before using the id and remove
direct accesses to pool.cache.
In `@tests/unit/test/temp_dir.h`:
- Around line 26-28: The TempDir constructor calls
llvm::sys::fs::createUniqueDirectory(prefix, root) but ignores the returned
error code so root may be empty on failure; modify the TempDir(llvm::StringRef
prefix) constructor to capture the returned llvm::ErrorCode (or std::error_code)
from createUniqueDirectory, assert or handle failure (e.g., assert(!ec, "Failed
to create temp directory") or log and abort) and ensure root is only used when
the call succeeds; reference TempDir, createUniqueDirectory, and root when
making the change.
- Around line 59-67: The touch() helper currently ignores filesystem errors and
may silently produce missing/empty files; update touch (and its use of path(),
llvm::sys::fs::create_directories and llvm::raw_fd_ostream) to detect and fail
on errors: check the result of create_directories and the std::error_code ec
from llvm::raw_fd_ostream, and if an error is present, fail the test (e.g., with
an assertion or throwing a test-fatal exception) including the path and
ec.message(); also ensure the output is properly flushed/closed after writing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44c435ab-928a-4ae3-9d3a-90dbb4895da2
📒 Files selected for processing (22)
.github/workflows/benchmark.ymlCMakeLists.txtbenchmarks/scan_benchmark.cppsrc/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/server/master_server.hsrc/support/path_pool.hsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.hsrc/syntax/include_resolver.cppsrc/syntax/include_resolver.hsrc/syntax/scan.cpptests/unit/compile/command_tests.cpptests/unit/syntax/dependency_graph_tests.cpptests/unit/syntax/include_resolver_tests.cpptests/unit/syntax/scan_tests.cpptests/unit/test/temp_dir.h
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>
Introduce IncludeResolver for fast header-path resolution using DirListingCache (readdir-based), DependencyGraph with BFS wavefront scanning (Phase 1: parallel read+scan, Phase 2: include resolution), and integrate scan_dependency_graph into MasterServer::load_workspace. Adds benchmark harness and comprehensive unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/dependency-graph
branch
from
March 27, 2026 05:34
5ad5af7 to
30f974b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/syntax/dependency_graph_tests.cpp (1)
371-371: Consider usingfind()instead ofoperator[]to avoid silent insertion.
pool.cache[tmp.path("src/main.cpp")]will insert a default entry (value 0) if the key doesn't exist, which could mask test failures if the path wasn't properly interned during scanning. Usingfind()orlookup()would make the test fail explicitly if the path is missing.Suggested improvement
- auto includes = graph.get_includes(pool.cache[tmp.path("src/main.cpp")], 0); + auto it = pool.cache.find(tmp.path("src/main.cpp")); + ASSERT_TRUE(it != pool.cache.end()); + auto includes = graph.get_includes(it->second, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/dependency_graph_tests.cpp` at line 371, The test currently uses pool.cache[tmp.path("src/main.cpp")] which will insert a default entry if the key is missing; change the access to use a non-mutating lookup (e.g., pool.cache.find(...) or pool.lookup(...) depending on API) and assert that the iterator/lookup result exists before calling graph.get_includes; specifically, locate the call that constructs includes (the line calling graph.get_includes with pool.cache[tmp.path("src/main.cpp")], 0) and replace the operator[] usage with a find()/lookup and explicit failure/assertion if the path is not found so the test fails loudly when the path wasn't interned.benchmarks/scan_benchmark.cpp (2)
249-255: Prefersetenv()overputenv()for safer environment variable handling.
putenv()does not copy the string—it expects the passed buffer to remain valid and unmodified for the program's lifetime. While thestatic std::stringensures persistence,setenv()is safer and more portable as it copies the value internally.Suggested fix
// Set UV_THREADPOOL_SIZE if not already set. // Use at least libuv's default (4) so low-core CI runners don't regress. if(!std::getenv("UV_THREADPOOL_SIZE")) { auto pool_size = std::max(hw_threads, 4u); - static std::string env = "UV_THREADPOOL_SIZE=" + std::to_string(pool_size); - putenv(env.data()); + setenv("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str(), 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 249 - 255, Replace the use of putenv for setting UV_THREADPOOL_SIZE with setenv to avoid relying on caller-owned buffers; after computing pool_size (from std::max(hw_threads, 4u)) call setenv("UV_THREADPOOL_SIZE", <stringified pool_size>, 1) so the C library copies the value and you don’t need the static std::string env; remove the static env variable and the putenv(env.data()) call and ensure you pass the stringified pool_size as the second argument to setenv and overwrite any existing value.
280-284: Guard against division by zero when CDB is empty.If
countis 0,unique_contexts.size()will also be 0, resulting in0.0/0.0which producesnanin the output.Suggested fix
- std::println("Context dedup: {} files -> {} unique contexts ({:.1f}x reduction)", - count, - unique_contexts.size(), - static_cast<double>(count) / unique_contexts.size()); + if(!unique_contexts.empty()) { + std::println("Context dedup: {} files -> {} unique contexts ({:.1f}x reduction)", + count, + unique_contexts.size(), + static_cast<double>(count) / unique_contexts.size()); + } else { + std::println("Context dedup: 0 files -> 0 unique contexts"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 280 - 284, The print currently divides by unique_contexts.size() which can be zero; modify the code around the std::println call to guard the division by computing a safe reduction value (e.g., double reduction = unique_contexts.empty() ? 0.0 : static_cast<double>(count) / unique_contexts.size()) and then use that reduction in the message (or print "N/A" when appropriate); refer to the variables count and unique_contexts and the std::println invocation to locate where to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@benchmarks/scan_benchmark.cpp`:
- Around line 249-255: Replace the use of putenv for setting UV_THREADPOOL_SIZE
with setenv to avoid relying on caller-owned buffers; after computing pool_size
(from std::max(hw_threads, 4u)) call setenv("UV_THREADPOOL_SIZE", <stringified
pool_size>, 1) so the C library copies the value and you don’t need the static
std::string env; remove the static env variable and the putenv(env.data()) call
and ensure you pass the stringified pool_size as the second argument to setenv
and overwrite any existing value.
- Around line 280-284: The print currently divides by unique_contexts.size()
which can be zero; modify the code around the std::println call to guard the
division by computing a safe reduction value (e.g., double reduction =
unique_contexts.empty() ? 0.0 : static_cast<double>(count) /
unique_contexts.size()) and then use that reduction in the message (or print
"N/A" when appropriate); refer to the variables count and unique_contexts and
the std::println invocation to locate where to make this change.
In `@tests/unit/syntax/dependency_graph_tests.cpp`:
- Line 371: The test currently uses pool.cache[tmp.path("src/main.cpp")] which
will insert a default entry if the key is missing; change the access to use a
non-mutating lookup (e.g., pool.cache.find(...) or pool.lookup(...) depending on
API) and assert that the iterator/lookup result exists before calling
graph.get_includes; specifically, locate the call that constructs includes (the
line calling graph.get_includes with pool.cache[tmp.path("src/main.cpp")], 0)
and replace the operator[] usage with a find()/lookup and explicit
failure/assertion if the path is not found so the test fails loudly when the
path wasn't interned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47c9f6ee-4574-4c3a-adda-9c20655ff849
📒 Files selected for processing (15)
.github/workflows/benchmark.ymlCMakeLists.txtbenchmarks/scan_benchmark.cppsrc/command/command.cppsrc/command/command.hsrc/server/master_server.cppsrc/server/master_server.hsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.hsrc/syntax/include_resolver.cppsrc/syntax/include_resolver.hsrc/syntax/scan.cpptests/unit/syntax/dependency_graph_tests.cpptests/unit/syntax/include_resolver_tests.cpptests/unit/syntax/scan_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- src/syntax/include_resolver.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/syntax/scan.cpp
- src/command/command.cpp
- src/server/master_server.cpp
- CMakeLists.txt
- src/server/master_server.h
- .github/workflows/benchmark.yml
- src/syntax/dependency_graph.cpp
The blacklist approach (exclude discarded/user_content/codegen) let warning flags, optimization levels, and other semantics-only options into the toolchain cache key. On LLVM's CDB (9300 files, 1236 contexts) this produced 53 unique keys instead of 6, causing 9x more BuildCompilation calls during prewarm — 736ms vs 45ms on Linux, and a timeout on Windows CI. Restore the original whitelist (is_toolchain_option) that only includes options affecting system path discovery: -target, -isysroot, --sysroot, -stdlib=, -gcc-toolchain, -nostdinc, -std=, -x. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/scan_benchmark.cpp`:
- Around line 249-250: The code reads auto runs = *opts.runs and then calls
vector::reserve with that value which can wrap if runs is non-positive; validate
that runs > 0 immediately after reading opts.runs and return or error out if
not, then cast runs once to size_t (e.g. size_t runs_sz =
static_cast<size_t>(runs)) and use runs_sz for the subsequent reserve() calls
(also update the other places mentioned around lines 297-300 where reserve() is
called) to avoid negative-to-unsigned wraparound and huge allocations.
In `@src/command/argument_parser.cpp`:
- Around line 118-134: is_toolchain_option() currently omits flags (e.g.,
-resource-dir, -m32/-m64, -arch, -nobuiltininc, -nostdlibinc and any other
toolchain-affecting args) that must be included in the cache-key built by
extract_toolchain_flags() and read by query_toolchain_cached(); update
is_toolchain_option() to include these specific options or, better, replace the
hard-coded switch with logic that derives the whitelist from Clang's option
groups / the ToolChain implementation (i.e., consult the same option categories
used by Clang/ToolChain to decide which flags affect cc1 and resource/header
discovery) so all flags that affect computed cc1 args are consistently included
in the cache key (refer to is_toolchain_option, extract_toolchain_flags, and
query_toolchain_cached to locate and verify correctness).
In `@src/syntax/dependency_graph.cpp`:
- Around line 325-327: The visited-set usage is wrong: scanned_files currently
keys only by path_id so a header scanned for one config will be skipped for
other configs; change the visit/cache scheme to use an IncludeKey-style key that
contains path_id and config_id (and include found_dir_idx in the key if you need
exact `#include_next` semantics) so the graph traversal calls like
graph.set_includes(header, other_config, ...) get enqueued for each config; keep
the path-stable scan_results caching by path_id for lexed data but switch
scanned_files (and the similar logic around the other occurrence at lines
~604-606) to record visits by the composite IncludeKey instead of plain path_id.
- Around line 271-273: The include_cache currently maps to only path_id, which
loses found_dir_idx on warm hits; change the cache value type used at the
local_include_cache/ext_cache->include_cache site to store the full resolution
tuple (path_id plus found_dir_idx) — e.g., a small struct or packed uint64 — and
update all lookup/insert sites (the variables include_cache, the code that
enqueues into scanned_files and next_wave, and the other affected blocks around
the later include-resolution logic) to read both path_id and found_dir_idx from
the cache so warm hits reuse the original found_dir_idx (so `#include_next`
resumes from the correct search directory). Ensure producers write both values
into the cache and consumers extract both values when pushing to scanned_files
and next_wave.
- Around line 683-695: The public function scan_dependency_graph currently
constructs its own et::event_loop, schedules scan_impl and blocks on loop.run,
which blocks the caller's event loop; instead remove the private loop and expose
an asynchronous entry point: change scan_dependency_graph to not run a local
loop but either (A) accept an external et::event_loop or executor and schedule
scan_impl on it, returning immediately (or returning a future/promise that
completes with ScanReport), or (B) spawn the scan onto a background
thread/worker and invoke a provided callback with the ScanReport; update callers
(e.g., MasterServer::load_workspace) to schedule or await the returned future
rather than block, and keep symbols scan_dependency_graph, scan_impl, ScanReport
and the loop scheduling logic consistent while eliminating loop.run from the
public API.
🪄 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: 979afd29-6426-4854-ad04-2f9fb3aa79ba
📒 Files selected for processing (7)
benchmarks/scan_benchmark.cppsrc/command/argument_parser.cppsrc/command/argument_parser.hsrc/command/command.cppsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.htests/unit/syntax/dependency_graph_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/syntax/dependency_graph.h
Print unique canonicals, patches, directories, and sample entries to help diagnose macOS dedup regression (9894/9895 unique contexts vs 1316/9882 on Ubuntu). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
benchmarks/scan_benchmark.cpp (4)
388-392: Lambda mutates input vector as a side effect.The
statslambda sorts the input vector in-place. While the vectors aren't used afterward, mutating inputs through a reference parameter is a subtle side effect that could cause issues if the code evolves.♻️ Take by value to avoid mutation
- auto stats = [](std::vector<std::int64_t>& v) { + auto stats = [](std::vector<std::int64_t> v) { std::ranges::sort(v); auto sum = std::accumulate(v.begin(), v.end(), std::int64_t{0}); return std::tuple{v.front(), sum / static_cast<std::int64_t>(v.size()), v.back()}; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 388 - 392, The stats lambda currently takes its parameter by reference and sorts the vector in-place, causing an unintended side effect; change the signature of the stats lambda to take the vector by value (e.g., std::vector<std::int64_t> v) so the sort and subsequent accumulate operate on a copy and do not mutate the caller's data, leaving the rest of the logic in the lambda (std::ranges::sort, std::accumulate, returning min/avg/max) unchanged.
254-258:putenvusage is correct but considersetenvfor clarity on POSIX.The
static std::stringensures the buffer persists for the process lifetime, whichputenvrequires. However,putenvstores the pointer directly and callers may modify the buffer through subsequentputenvcalls on the same variable.For a benchmark tool this is acceptable. On POSIX systems,
setenv("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str(), 0)would be cleaner as it copies the value internally, butputenvworks cross-platform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 254 - 258, Replace the current putenv-based UV_THREADPOOL_SIZE setting to use POSIX setenv when available: instead of relying on static std::string env and putenv(env.data()), call setenv("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str(), 0) (or use an `#ifdef` to fall back to the existing putenv approach on non-POSIX platforms). Update the logic around getenv, pool_size, and env so the value is copied internally (avoiding reliance on env.data() lifetime) and only use putenv as the fallback.
279-302: Pointer-based uniqueness relies on string interning; comment is misleading.Line 293 says "Count unique patches by hashing their content," but the code compares
const char*pointers, not string contents. This works correctly only if the compilation database interns all strings (same content → same address).If interning is incomplete, identical strings at different addresses will be counted as distinct, inflating the unique count. For diagnostic purposes this may be acceptable if you're specifically measuring interning effectiveness. Otherwise, consider string-based comparison.
♻️ Content-based comparison if needed
- std::set<const char*> unique_dirs; + std::set<std::string_view> unique_dirs; ... - // Count unique patches by hashing their content. - std::set<std::vector<const char*>> unique_patches; + // Count unique patches by their string content. + std::set<std::vector<std::string_view>> unique_patches; for(auto& entry: cdb.get_entries()) { unique_patches.insert( - std::vector<const char*>(entry.info->patch.begin(), entry.info->patch.end())); + std::vector<std::string_view>(entry.info->patch.begin(), entry.info->patch.end())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 279 - 302, The current unique_patches logic collects std::vector<const char*> (from entry.info->patch) which only compares pointer identities (string interning assumption) rather than content; replace the pointer-based set with a content-based representation (e.g., build std::vector<std::string> or std::string_view-backed vector for each entry or compute a combined hash of the concatenated patch strings) and insert that into unique_patches so equality is determined by actual patch text; update the unique_patches declaration and the insert call that references entry.info->patch to construct and store content-based vectors instead of vectors of const char*.
92-95: Consider adding bounds validation forraw_id.The
raw_idis extracted by masking withPATH_ID_MASK(31 bits). Whilescan_dependency_graphshould only produce valid IDs, a corrupted graph entry could cause out-of-bounds access. For a benchmark tool this is acceptable, but a defensive check would improve robustness.🛡️ Optional defensive check
for(auto flagged_id: inc_ids) { auto raw_id = flagged_id & DependencyGraph::PATH_ID_MASK; + if(raw_id >= path_pool.paths.size()) { + std::println(stderr, "Warning: invalid path_id {} in graph", raw_id); + continue; + } node.includes.push_back(path_pool.paths[raw_id].str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 92 - 95, The loop extracts raw_id by masking with DependencyGraph::PATH_ID_MASK and then indexes path_pool.paths[raw_id] without verifying raw_id is within path_pool.paths.size(); add a defensive bounds check (e.g., if(raw_id >= path_pool.paths.size()) continue or log+continue) before pushing into node.includes to avoid out-of-bounds access in case of corrupted entries; update the loop around inc_ids where raw_id is computed and use the guarded access to path_pool.paths and its .str() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@benchmarks/scan_benchmark.cpp`:
- Around line 388-392: The stats lambda currently takes its parameter by
reference and sorts the vector in-place, causing an unintended side effect;
change the signature of the stats lambda to take the vector by value (e.g.,
std::vector<std::int64_t> v) so the sort and subsequent accumulate operate on a
copy and do not mutate the caller's data, leaving the rest of the logic in the
lambda (std::ranges::sort, std::accumulate, returning min/avg/max) unchanged.
- Around line 254-258: Replace the current putenv-based UV_THREADPOOL_SIZE
setting to use POSIX setenv when available: instead of relying on static
std::string env and putenv(env.data()), call setenv("UV_THREADPOOL_SIZE",
std::to_string(pool_size).c_str(), 0) (or use an `#ifdef` to fall back to the
existing putenv approach on non-POSIX platforms). Update the logic around
getenv, pool_size, and env so the value is copied internally (avoiding reliance
on env.data() lifetime) and only use putenv as the fallback.
- Around line 279-302: The current unique_patches logic collects
std::vector<const char*> (from entry.info->patch) which only compares pointer
identities (string interning assumption) rather than content; replace the
pointer-based set with a content-based representation (e.g., build
std::vector<std::string> or std::string_view-backed vector for each entry or
compute a combined hash of the concatenated patch strings) and insert that into
unique_patches so equality is determined by actual patch text; update the
unique_patches declaration and the insert call that references entry.info->patch
to construct and store content-based vectors instead of vectors of const char*.
- Around line 92-95: The loop extracts raw_id by masking with
DependencyGraph::PATH_ID_MASK and then indexes path_pool.paths[raw_id] without
verifying raw_id is within path_pool.paths.size(); add a defensive bounds check
(e.g., if(raw_id >= path_pool.paths.size()) continue or log+continue) before
pushing into node.includes to avoid out-of-bounds access in case of corrupted
entries; update the loop around inc_ids where raw_id is computed and use the
guarded access to path_pool.paths and its .str() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c81358bb-cb6b-4cb1-8062-98b279360819
📒 Files selected for processing (1)
benchmarks/scan_benchmark.cpp
Show full per-arg diff between two entries sharing the same canonical but different patches, to identify what causes per-file uniqueness on macOS/Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
benchmarks/scan_benchmark.cpp (2)
92-95: Consider adding bounds check forraw_idbefore array access.While
raw_idshould always be valid if the graph was built correctly with the samePathPool, a defensive bounds check prevents crashes from data corruption or logic bugs elsewhere.🛡️ Suggested defensive check
for(auto flagged_id: inc_ids) { auto raw_id = flagged_id & DependencyGraph::PATH_ID_MASK; + if(raw_id >= path_pool.paths.size()) { + std::println(stderr, "Warning: invalid include id {} in graph", raw_id); + continue; + } node.includes.push_back(path_pool.paths[raw_id].str()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 92 - 95, The loop that extracts raw_id from flagged_id using DependencyGraph::PATH_ID_MASK then accesses path_pool.paths[raw_id] is unsafe; add a defensive bounds check on raw_id against path_pool.paths.size() before pushing to node.includes to avoid out-of-range access. Locate the loop that iterates over inc_ids and the variables flagged_id, raw_id, node.includes and if raw_id is invalid either skip that entry (and optionally log or count the corruption) or push a safe placeholder, ensuring no direct access to path_pool.paths[raw_id] when raw_id >= path_pool.paths.size().
254-258:putenvis POSIX-specific; considersetenvor cross-platform alternative.
putenvworks on POSIX systems but Windows uses_putenvor_putenv_s. Since this benchmark might run on Windows (mentioned in PR objectives for macOS/Windows path uniqueness), consider using a cross-platform approach.🔧 Cross-platform alternative
if(!std::getenv("UV_THREADPOOL_SIZE")) { auto pool_size = std::max(hw_threads, 4u); - static std::string env = "UV_THREADPOOL_SIZE=" + std::to_string(pool_size); - putenv(env.data()); +#ifdef _WIN32 + _putenv_s("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str()); +#else + setenv("UV_THREADPOOL_SIZE", std::to_string(pool_size).c_str(), 0); +#endif }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/scan_benchmark.cpp` around lines 254 - 258, The code uses putenv to set UV_THREADPOOL_SIZE which is POSIX-specific; replace it with a cross-platform branch that uses setenv on POSIX and _putenv_s on Windows: build the pool_size string as before, then inside an `#ifdef` _WIN32 use _putenv_s("UV_THREADPOOL_SIZE", value.c_str()); else use setenv("UV_THREADPOOL_SIZE", value.c_str(), 1); (remove the static env hack) and include the needed headers; modify the block around pool_size/putenv to use these platform-specific calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/scan_benchmark.cpp`:
- Around line 269-275: Check the result of CompilationDatabase::load (the
variable count) after calling cdb.load(cdb_path) and bail out if it indicates
failure (e.g., count == 0) instead of continuing; update the code around
CompilationDatabase, cdb.load, cdb_path and the subsequent std::println call to
detect an empty/failed load, print a clear error mentioning cdb_path and the
load failure, and exit or return a non-zero status so the benchmark does not run
on an empty database.
---
Nitpick comments:
In `@benchmarks/scan_benchmark.cpp`:
- Around line 92-95: The loop that extracts raw_id from flagged_id using
DependencyGraph::PATH_ID_MASK then accesses path_pool.paths[raw_id] is unsafe;
add a defensive bounds check on raw_id against path_pool.paths.size() before
pushing to node.includes to avoid out-of-range access. Locate the loop that
iterates over inc_ids and the variables flagged_id, raw_id, node.includes and if
raw_id is invalid either skip that entry (and optionally log or count the
corruption) or push a safe placeholder, ensuring no direct access to
path_pool.paths[raw_id] when raw_id >= path_pool.paths.size().
- Around line 254-258: The code uses putenv to set UV_THREADPOOL_SIZE which is
POSIX-specific; replace it with a cross-platform branch that uses setenv on
POSIX and _putenv_s on Windows: build the pool_size string as before, then
inside an `#ifdef` _WIN32 use _putenv_s("UV_THREADPOOL_SIZE", value.c_str()); else
use setenv("UV_THREADPOOL_SIZE", value.c_str(), 1); (remove the static env hack)
and include the needed headers; modify the block around pool_size/putenv to use
these platform-specific calls.
🪄 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: 6565571b-c2eb-4b25-ac7e-ecbb9026269e
📒 Files selected for processing (1)
benchmarks/scan_benchmark.cpp
On macOS, source file paths starting with /Users/... are misinterpreted by clang's option parser as the MSVC-style /U (undefine) flag. This caused the source file path to leak into per-file patch args, making every CompilationInfo unique and destroying context deduplication (9895 unique contexts instead of ~1300). Detect the misparse: if an option with "/" prefix + its value reconstructs to an absolute path, discard it as a file path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lity mask On macOS/Linux, clang's option parser interprets Unix absolute paths like /Users/... as MSVC-style /U sers/... (undefine macro), breaking context deduplication (9895/9896 unique contexts instead of ~1300). Use ParseOneArg's Visibility parameter to exclude CLOption-tagged options when the driver is not cl.exe. This is the proper solution instead of the hacky path-reconstruction check. Also clean up verbose diagnostic code in scan_benchmark.cpp. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Temporarily dump canonical command details when dedup is poor (>200 unique canonicals) to identify what per-file arg leaks into canonical on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
clang-cl.exe needs CLOption visibility just like cl.exe, otherwise MSVC-style /TP, /EHs, /Oi options get misparsed and leak per-file garbage into canonical commands, breaking dedup on Windows (9811 unique canonicals instead of ~76). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Skip .rc (Windows resource), .asm, .def and other non-C-family entries that build systems like CMake emit into compile_commands.json. Uses clang::driver::types to check file extensions. This removes ~450 cmake_llvm_rc and ml64 entries on Windows that were polluting the canonical dedup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On Windows with clang-cl, /Fd (PDB path) varies per-target and -- is parsed as OPT_UNKNOWN. Both leak into canonical, breaking dedup. Also discard /Fo (MSVC output file) for consistency with -o. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On Windows with clang-cl, CMake generates compile_commands.json entries ending with `-- <source-file>`. The `--` option is parsed as OPT__DASH_DASH (KIND_REMAINING_ARGS), consuming the source file path as its value. Both leaked into the canonical command, making every entry unique (1.0x dedup, 9807 unique canonicals out of 9812 entries). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Deduplicate config_id in DependencyGraph::set_includes to prevent accumulation on repeated scans - Store found_dir_idx in include_cache (CachedInclude struct) so that #include_next resolves correctly for cache-hit headers - Validate --runs > 0 in benchmark to prevent reserve() overflow - Add ".\\" check in needs_normalize for Windows #include paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the same header is included across multiple configs — unconditionally in one, conditionally in another — the merge now correctly strips the CONDITIONAL_FLAG. Previously, whichever config was iterated first won, producing non-deterministic results. Also add TODOs for future improvements: - ScanCache generation counter for safe partial invalidation - DirListingCache per-directory invalidation and case-insensitive FS note - pop_back() assertion for temp source file invariant - Test coverage: circular includes, warm ScanCache, #include_next edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-by-zero - default_visibility: strip trailing version digits (e.g. clang-cl-17) before checking for cl/clang-cl, matching driver_family() logic. Previously versioned clang-cl drivers lost all MSVC-style options. - resolve_dir: log readdir failures at DEBUG level instead of silently caching an empty directory listing. - benchmark: guard against division by zero when CDB has no entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Log dependency scan summary (timing, file counts, edge counts, resolution accuracy) at INFO level during workspace load. Warn when there are unresolved includes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 28, 2026
This was referenced Apr 6, 2026
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a dependency graph scanner that discovers the full include-graph of a project at startup, without invoking the compiler. The scanner reads every source file from the compilation database, lexer-scans it for
#includeandimportdirectives, resolves each include to a filesystem path, and transitively follows discovered headers using a wavefront BFS. The result is aDependencyGraphmapping every file to its direct includes (keyed by file and search-config pair), plus module-name-to-file mappings for C++20 modules.This replaces per-file on-demand header discovery with a one-shot upfront scan, giving the server a complete picture of the project's dependency structure before any LSP request arrives.
Architecture
Core components
DependencyGraph(src/syntax/dependency_graph.h/.cpp)(path_id, config_id) -> [path_id...]using LLVM DenseMaps with a customIncludeKeyhash.CONDITIONAL_FLAG = 0x80000000) indicating whether the include was inside an#if/#ifdefblock.get_all_includes()merges edges across all configs for a file, with unconditional edges winning over conditional ones.module_name -> [path_id].IncludeResolver(src/syntax/include_resolver.h/.cpp)#includedirectives to filesystem paths using a directory listing cache (DirListingCache) instead of per-filestat()calls.readdir()and the resulting filenames are stored in anllvm::StringSet. File existence checks become O(1) hash lookups."llvm/Support/raw_ostream.h", a two-stage strategy is used: first check if the top-level component (llvm) exists in the pre-listed directory, then only resolve the subdirectory on match.#include_nextsupport.ResolvedSearchConfigpre-resolves all search directory StringMap entries to direct pointers, eliminating map lookups during hot resolution loops.Wavefront BFS scanner (
scan_dependency_graph()independency_graph.cpp)eventideevent loop with a libuv thread pool.#includeto a path, intern it in PathPool, build graph edges, and collect newly discovered files for the next wave.Key optimizations
readdir()on worker threads concurrently with Wave 0's file scanning. The dir cache await happens before Phase 2, so the cost ismax(dir_time, scan_time)rather thandir_time + scan_time.<...>includes are cached per(config_id, header_name)since they don't depend on the includer's directory. This eliminates redundant directory searches for commonly included system/library headers.ScanReportprovides detailed timing and accuracy metrics: per-wave breakdowns, per-phase wall-clock times, cumulative I/O statistics, readdir cache hit rates, and a list of unresolved includes.Cross-platform fixes
MSVC/clang-cl option visibility (
argument_parser.h/.cpp,command.cpp,search_config.cpp)default_visibility()that detectscl.exe/clang-cldrivers (including versioned names likeclang-cl-17) and returns an appropriate visibility mask.CLOptionvisibility group is excluded. This prevents Unix absolute paths like/Users/...from being misparsed as MSVC/U sers/...options on macOS/Linux.ParseOneArg()and set before parsing inCompilationDatabase::load(), toolchain extraction, andextract_search_config().Non-C-family file filtering (
argument_parser.h/.cpp,command.cpp)is_c_family_file()uses clang'stypes::lookupTypeForExtensionto detect C/C++/ObjC/CUDA files.CompilationDatabase::load()now skips.rc,.asm,.def, and other non-C-family entries that some build systems emit intocompile_commands.json.Toolchain option whitelist (
argument_parser.cpp)is_toolchain_option()defines a positive whitelist of flags (--target,--sysroot,-std=,-x,-nostdinc, etc.) that affect system include path discovery.Additional discarded options:
OPT_UNKNOWN,OPT__DASH_DASH,OPT__SLASH_Fo, andOPT__SLASH_Fdare now filtered during argument parsing.Server integration
MasterServernow holds aDependencyGraphmember and callsscan_dependency_graph()duringload_workspace(), immediately after loading the CDB.CompilationDatabase::intern_path()andget_entries()expose internals needed by the scanner.Benchmark
benchmarks/scan_benchmark.cppis a standalone CLI tool that:compile_commands.jsonand runsscan_dependency_graph()N times (default 20) as true cold starts (CDB, PathPool, and DependencyGraph are rebuilt each iteration).--export graph.jsonto dump the full dependency graph..github/workflows/benchmark.ymlruns on PRs against main, on ubuntu-24.04 / macos-15 / windows-2025. It clones LLVM, generates a CDB with all major subprojects enabled, and runs 20 cold-start iterations.Files changed
src/syntax/dependency_graph.{h,cpp}DependencyGraphclass,ScanReport,ScanCache, wavefront BFS scannersrc/syntax/include_resolver.{h,cpp}DirListingCache,ResolvedSearchConfig, include resolution with multi-component path optimizationsrc/command/argument_parser.{h,cpp}src/command/command.{h,cpp}intern_path()/get_entries()APIssrc/command/search_config.cppsrc/server/master_server.{h,cpp}DependencyGraphmember, startup scan + log reportsrc/syntax/scan.cppStringSet.hincludebenchmarks/scan_benchmark.cpp.github/workflows/benchmark.ymltests/unit/syntax/CMakeLists.txtclice-core, addscan_benchmarktargetTest plan
dependency_graph_tests.cpp: unit tests for module mapping, include edges (per-config, cross-config union, conditional flag semantics), file/module/edge countsdependency_graph_tests.cpp: integration tests usingTempDirwith real files — single file, chain includes, diamond includes, conditional includes,#include_next, multi-config, unresolved includesinclude_resolver_tests.cpp: quoted vs angled resolution order, multi-component path optimization,#include_nextskipping, absolute paths,../relative paths, missing directories, empty search configscan_tests.cpp: verifyis_angledflag is correctly set for<...>vs"..."includes🤖 Generated with Claude Code