Repository navigation
refactor: introduce syntax/scan module with DependencyDirectivesGetter - #357
Merged
Merged
Conversation
- Introduce `syntax/scan.h` with lexer-based `scan()` for fast module name and include extraction, and `scan_with_preprocessor()` for full preprocessing-based dependency scanning with hooked VFS - Move `compute_preamble_bound(s)` from `compile/preamble` to `syntax/scan` - Consolidate `PCHInfo`, `ModuleInfo`, `PCMInfo` into `compile/compilation.h`, removing `compile/preamble.h`, `compile/preamble.cpp`, `compile/module.h` - Remove server code (`src/server/`) pending redesign; replace `clice.cc` with placeholder
|
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:
📝 WalkthroughWalkthroughRemoves the JSON‑RPC server/worker subsystem and runtime types, consolidates compilation types into Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@src/clice.cc`:
- Around line 1-2: The current main() in src/clice.cc silently returns success;
change it to fail loudly or stop installing the binary: update main to print a
clear error to stderr (via fprintf or std::cerr) and return a non-zero exit code
(e.g., 1) indicating the CLI is unavailable, or temporarily remove the
install/packaging of the clice target until the redesigned CLI is implemented;
locate the function main in src/clice.cc to make this change.
In `@src/compile/compilation.h`:
- Around line 39-40: PCHInfo::arguments currently holds borrowed const char*
pointers (populated from params.arguments which comes from CompilationDatabase's
StringSet), creating a fragile lifetime dependency; fix by changing
PCHInfo::arguments from std::vector<const char*> to std::vector<std::string> and
update all construction sites that populate arguments (e.g., where
params.arguments is copied) to emplace_back or assign owned strings so PCHInfo
no longer depends on CompilationDatabase lifetime; if you opt not to own
strings, instead add a clear comment on the PCHInfo struct documenting that
arguments are borrowed and valid only while the originating
CompilationDatabase/StringSet remains alive and ensure no caching persists
PCHInfo beyond that lifetime.
In `@src/syntax/scan.cpp`:
- Around line 79-115: When handling a `module` token inside the block that sets
result.module_name and result.is_interface_unit, detect the special private
fragment form `module :private;` and skip it instead of treating `:private` as a
module name: after reading `next = lexer.next()` and before collecting
identifiers, check if the next non-eof token sequence is a colon token followed
by an identifier with text "private" and then a semicolon; if so, advance the
lexer past those tokens (like you do for the `module;` case) and continue
without modifying result.module_name or result.is_interface_unit. Ensure you
reference the existing symbols lexer.next(), lexer.advance(), tok.kind,
tok.is_identifier(), tok.text(content), result.module_name, and
result.is_interface_unit when implementing this check.
- Around line 183-221: IncludeOnlyVFS currently strips every file (via
openFileForRead -> strip_to_includes), which removes module/import tokens from
the primary input; change IncludeOnlyVFS so it does not strip the primary input:
add a member (e.g., PrimaryInputPath) set via the constructor or setter, and in
openFileForRead compare the incoming path (path_str) to PrimaryInputPath and if
equal return the underlying file buffer unchanged (bypass strip_to_includes and
InMemoryFile wrapping) while preserving the existing visited check/underlying FS
error handling for other files; update the constructor signature and call sites
that create IncludeOnlyVFS to pass the primary input path.
- Around line 243-245: The current include collection uses
file->getFileEntry().tryGetRealPathName().str() and can insert an empty string
for virtual/remapped/builtin headers; update the logic around
result.includes.emplace_back(...) so it checks the FileEntry's
tryGetRealPathName() result and if that string is empty uses
file->getFileEntry().getName() as a fallback before emplacing. Locate the block
around file->getFileEntry() / result.includes.emplace_back in scan.cpp and
implement the conditional fallback to preserve dependency names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d6a5ce8-8a2c-4f35-9bd4-28cdeb0e1633
📒 Files selected for processing (13)
CMakeLists.txtsrc/clice.ccsrc/compile/compilation.hsrc/compile/module.hsrc/compile/preamble.cppsrc/compile/preamble.hsrc/server/protocol.hsrc/server/runtime.hsrc/server/server.cppsrc/server/worker.cppsrc/syntax/scan.cppsrc/syntax/scan.htests/unit/test/tester.cpp
💤 Files with no reviewable changes (7)
- src/compile/preamble.h
- src/server/runtime.h
- src/compile/module.h
- src/server/worker.cpp
- src/server/protocol.h
- src/server/server.cpp
- src/compile/preamble.cpp
…can module Replace the custom VFS hook (IncludeOnlyVFS/strip_to_includes) with clang's native DependencyDirectivesGetter + scanSourceForDependencyDirectives API. - Rewrite scan() to use scanSourceForDependencyDirectives instead of custom Lexer - Split scan_with_preprocessor into scan_fuzzy (strips conditionals/#define, processes all #includes unconditionally) and scan_precise (keeps all directives) - scan_fuzzy returns StringMap<ScanResult> with per-file include results - Add SharedScanCache for cross-file directive caching with include_is_conditional flags computed from raw directive structure before filtering - Add IncludeInfo struct with conditional and not_found fields - Add content parameter to scan_fuzzy/scan_precise for remapped file support via OverlayFileSystem - Override FileNotFound in fuzzy mode to suppress errors and continue scanning - Use std::deque for local_entries to avoid Directive::Tokens ArrayRef invalidation - Add 19 unit tests covering scan, scan_fuzzy, and scan_precise Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Test all 3 OS (windows, ubuntu, macos) x 2 build types (Debug, RelWithDebInfo) without exclusions, matching the xmake matrix coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 8 test cases for compute_preamble_bound and compute_preamble_bounds covering empty input, no directives, single/multiple includes, global module fragment, bounds vector ordering, stopping at code, and conditional directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use simple git diff check instead of reviewdog action for format verification. Remove unnecessary PR write permissions from format job. Also apply clang-format and use raw string literals in preamble tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/syntax/scan.cpp (1)
76-108:⚠️ Potential issue | 🟠 MajorHandle
module :private;(private module fragment) to avoid overwriting the primary module name.When a primary interface unit contains a private module fragment (
module :private;), the current code at lines 97-103 collects:privateas the module name, overwriting the actual module name from the earlierexport moduledeclaration. The private module fragment should be skipped.🐛 Proposed fix
case dds::cxx_module_decl: case dds::cxx_export_module_decl: { if(conditional_depth > 0) { result.need_preprocess = true; return result; } // Collect module name from tokens: skip keywords, then // collect identifiers, '.', ':'. std::string module_name; bool seen_module_keyword = false; + bool is_private_fragment = false; for(auto& tok: dir.Tokens) { if(!seen_module_keyword) { if(tok.is(clang::tok::raw_identifier)) { auto spelling = content.substr(tok.Offset, tok.Length); if(spelling == "module") { seen_module_keyword = true; } } continue; } + // Check for private module fragment: `module :private;` + if(tok.is(clang::tok::colon) && module_name.empty()) { + // Peek ahead - if next identifier is "private", skip this directive + is_private_fragment = true; + continue; + } + if(is_private_fragment && tok.is(clang::tok::raw_identifier)) { + auto spelling = content.substr(tok.Offset, tok.Length); + if(spelling == "private") { + // This is `module :private;` - skip entirely + is_private_fragment = false; + module_name.clear(); + break; + } + // Not "private", restore the colon + is_private_fragment = false; + module_name += ':'; + } if(tok.is(clang::tok::raw_identifier)) { module_name += content.substr(tok.Offset, tok.Length); } else if(tok.is(clang::tok::period)) { module_name += '.'; } else if(tok.is(clang::tok::colon)) { module_name += ':'; } } - result.module_name = std::move(module_name); - result.is_interface_unit = (dir.Kind == dds::cxx_export_module_decl); + // Only update if we got a valid module name (not private fragment) + if(!module_name.empty()) { + result.module_name = std::move(module_name); + result.is_interface_unit = (dir.Kind == dds::cxx_export_module_decl); + } break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/scan.cpp` around lines 76 - 108, The loop that builds module_name (iterating dir.Tokens with seen_module_keyword) incorrectly treats the private fragment "module :private;" as part of the module name; update the token-consumption logic in that loop (the block handling dir.Tokens, seen_module_keyword, and module_name) to detect and skip a private module fragment: if immediately after seeing the "module" keyword you encounter a ':' token followed by the identifier "private" and then a ';' (or equivalent token sequence), do not append ':' or "private" to module_name and skip that fragment (continue to the next top-level token / break out of the token loop as appropriate) so the previously captured primary module name is preserved. Ensure result.module_name and result.is_interface_unit semantics remain unchanged.
🧹 Nitpick comments (1)
tests/unit/syntax/scan_tests.cpp (1)
303-363: Consider adding test coverage for module detection inscan_precise.The
scan_precisetests verify include behavior but don't test the module-related fields (module_name,is_interface_unit) that are populated frompp.isInNamedModule()at lines 584-587 inscan.cpp. Additionally, consider adding a test for themodulesvector population frommoduleImportcallbacks.💡 Suggested additional test case
TEST_CASE(PreciseModuleDetection) { auto vfs = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>(); vfs->addFile("/test/main.cpp", 0, llvm::MemoryBuffer::getMemBuffer(R"( export module my.module; import other.module; )")); const char* args[] = {"clang++", "-std=c++20", "/test/main.cpp"}; auto result = scan_precise(args, "/test", false, {}, nullptr, vfs); EXPECT_EQ(result.module_name, "my.module"); EXPECT_TRUE(result.is_interface_unit); ASSERT_EQ(result.modules.size(), 1u); EXPECT_EQ(result.modules[0], "other.module"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/scan_tests.cpp` around lines 303 - 363, Add a new unit test to cover module detection in scan_precise: create a VFS file for main.cpp containing "export module my.module; import other.module;", call scan_precise (same args pattern used in other tests), and assert that result.module_name == "my.module", result.is_interface_unit is true, and result.modules contains "other.module"; this exercises the code paths that read pp.isInNamedModule() and the moduleImport callbacks so module_name, is_interface_unit, and modules are populated.
🤖 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/syntax/scan.cpp`:
- Around line 525-529: The code calls instance->getFrontendOpts().Inputs[0]
without checking Inputs size, risking undefined behavior when Inputs is empty;
update the BeginSourceFile calls (in the function using
clang::PreprocessOnlyAction and in scan_precise) to first check that
instance->getFrontendOpts().Inputs is not empty (e.g., check Inputs.empty() and
handle the error by returning results or logging) before dereferencing
Inputs[0], ensuring safe early exit if there are no inputs.
- Around line 379-391: The current inclusion resolution in
PreciseScanPPCallbacks stores tryGetRealPathName() into resolved_path and uses
that even when it's empty, losing virtual/remapped or builtin header names;
update the logic where resolved_path is assigned (the block using
file->getFileEntry().tryGetRealPathName().str()) to fall back to
file->getFileEntry().getName().str() when tryGetRealPathName() returns an empty
string (mirror the behavior in FuzzyScanPPCallbacks), so resolved_path becomes
the real path if present otherwise the file entry name before pushing into
result.includes.
---
Duplicate comments:
In `@src/syntax/scan.cpp`:
- Around line 76-108: The loop that builds module_name (iterating dir.Tokens
with seen_module_keyword) incorrectly treats the private fragment "module
:private;" as part of the module name; update the token-consumption logic in
that loop (the block handling dir.Tokens, seen_module_keyword, and module_name)
to detect and skip a private module fragment: if immediately after seeing the
"module" keyword you encounter a ':' token followed by the identifier "private"
and then a ';' (or equivalent token sequence), do not append ':' or "private" to
module_name and skip that fragment (continue to the next top-level token / break
out of the token loop as appropriate) so the previously captured primary module
name is preserved. Ensure result.module_name and result.is_interface_unit
semantics remain unchanged.
---
Nitpick comments:
In `@tests/unit/syntax/scan_tests.cpp`:
- Around line 303-363: Add a new unit test to cover module detection in
scan_precise: create a VFS file for main.cpp containing "export module
my.module; import other.module;", call scan_precise (same args pattern used in
other tests), and assert that result.module_name == "my.module",
result.is_interface_unit is true, and result.modules contains "other.module";
this exercises the code paths that read pp.isInNamedModule() and the
moduleImport callbacks so module_name, is_interface_unit, and modules are
populated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10b4b8c4-ddeb-49a8-a7b3-061bb7d60f20
📒 Files selected for processing (3)
src/syntax/scan.cppsrc/syntax/scan.htests/unit/syntax/scan_tests.cpp
16bit-ykiko
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/syntax/scan.cpp (3)
526-530:⚠️ Potential issue | 🟡 MinorAdd defensive check for empty
Inputsbefore accessing index 0.
instance->getFrontendOpts().Inputs[0]is accessed without verifying the vector is non-empty. Whilecreate_scan_instancetypically ensures an input file from arguments, malformed command lines could result in an emptyInputsvector, causing undefined behavior.🛡️ Proposed defensive check
auto action = std::make_unique<clang::PreprocessOnlyAction>(); + if(instance->getFrontendOpts().Inputs.empty()) { + return results; + } + if(!action->BeginSourceFile(*instance, instance->getFrontendOpts().Inputs[0])) { return results; }The same check should be added in
scan_preciseat line 573.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/scan.cpp` around lines 526 - 530, Add defensive checks that the frontend Inputs vector is non-empty before dereferencing index 0: before calling action->BeginSourceFile(*instance, instance->getFrontendOpts().Inputs[0]) (in the block where PreprocessOnlyAction is used) verify instance->getFrontendOpts().Inputs.empty() is false and return results (or handle error) if empty; apply the same guard in scan_precise where the same Inputs[0] access occurs. Ensure the checks are placed immediately before each BeginSourceFile call so you never read Inputs[0] when the vector is empty.
378-391:⚠️ Potential issue | 🟠 MajorAdd fallback to
getName()whentryGetRealPathName()is empty.
PreciseScanPPCallbacksdoesn't fall back togetName()whentryGetRealPathName()returns an empty string, unlikeFuzzyScanPPCallbacks(lines 315-318). This can silently lose dependency information for virtual/remapped files and builtin headers.🐛 Proposed fix for consistency with FuzzyScanPPCallbacks
bool not_found = !file.has_value(); std::string resolved_path; if(file) { - resolved_path = file->getFileEntry().tryGetRealPathName().str(); + resolved_path = file->getFileEntry().tryGetRealPathName().str(); + if(resolved_path.empty()) { + resolved_path = file->getName().str(); + } } else { resolved_path = file_name.str(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/syntax/scan.cpp` around lines 378 - 391, In PreciseScanPPCallbacks (the block that builds resolved_path using file->getFileEntry().tryGetRealPathName().str()), add a fallback to use file->getFileEntry().getName() when tryGetRealPathName() yields an empty string (mirror the behavior in FuzzyScanPPCallbacks). Concretely, after calling tryGetRealPathName().str() check if resolved_path.empty() and if so assign resolved_path = file->getFileEntry().getName().str(); leave the existing else branch (using file_name.str()) unchanged for the case where file is not present.
76-108:⚠️ Potential issue | 🟠 MajorMissing handling for
module :private;private module fragment.The module name collection logic doesn't distinguish the private module fragment (
module :private;) from regular module declarations. When a file contains bothexport module foo;andmodule :private;, the latter overwritesresult.module_namewith:privateand resetsis_interface_unittofalse.After detecting the
modulekeyword (line 91-93), check if the next token sequence is:private;and skip it without modifying the result.🐛 Proposed fix
if(spelling == "module") { seen_module_keyword = true; + // Check for private module fragment: module :private; + // Skip it without modifying result. + auto remaining = llvm::ArrayRef(dir.Tokens).drop_front( + &tok - dir.Tokens.begin() + 1); + if(remaining.size() >= 2 && + remaining[0].is(clang::tok::colon) && + remaining[1].is(clang::tok::raw_identifier)) { + auto ident = content.substr(remaining[1].Offset, remaining[1].Length); + if(ident == "private") { + goto next_directive; // Skip this directive + } + } }Note: Consider using a cleaner control flow pattern than
goto, such as a flag variable or restructuring the loop.
🧹 Nitpick comments (1)
tests/unit/syntax/scan_tests.cpp (1)
359-367: Consider using explicit position checks for clarity.The assertion
EXPECT_TRUE(bound <= src.find("int"))relies on the string being present. While the test source is carefully constructed, usingASSERT_NE(src.find("int"), llvm::StringRef::npos)before the comparison would make the test more robust and self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/scan_tests.cpp` around lines 359 - 367, In TEST_CASE SingleInclude, make the preamble bound check more robust by first asserting the "int" token is present: add an assertion that src.find("int") is not llvm::StringRef::npos (e.g., using ASSERT_NE) before calling EXPECT_TRUE(bound <= src.find("int")), so compute_preamble_bound and the subsequent comparison only run when the position is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/syntax/scan.cpp`:
- Around line 526-530: Add defensive checks that the frontend Inputs vector is
non-empty before dereferencing index 0: before calling
action->BeginSourceFile(*instance, instance->getFrontendOpts().Inputs[0]) (in
the block where PreprocessOnlyAction is used) verify
instance->getFrontendOpts().Inputs.empty() is false and return results (or
handle error) if empty; apply the same guard in scan_precise where the same
Inputs[0] access occurs. Ensure the checks are placed immediately before each
BeginSourceFile call so you never read Inputs[0] when the vector is empty.
- Around line 378-391: In PreciseScanPPCallbacks (the block that builds
resolved_path using file->getFileEntry().tryGetRealPathName().str()), add a
fallback to use file->getFileEntry().getName() when tryGetRealPathName() yields
an empty string (mirror the behavior in FuzzyScanPPCallbacks). Concretely, after
calling tryGetRealPathName().str() check if resolved_path.empty() and if so
assign resolved_path = file->getFileEntry().getName().str(); leave the existing
else branch (using file_name.str()) unchanged for the case where file is not
present.
---
Nitpick comments:
In `@tests/unit/syntax/scan_tests.cpp`:
- Around line 359-367: In TEST_CASE SingleInclude, make the preamble bound check
more robust by first asserting the "int" token is present: add an assertion that
src.find("int") is not llvm::StringRef::npos (e.g., using ASSERT_NE) before
calling EXPECT_TRUE(bound <= src.find("int")), so compute_preamble_bound and the
subsequent comparison only run when the position is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbf96587-c6ae-4c28-9c67-c190af088917
📒 Files selected for processing (5)
.github/workflows/check-format.yml.github/workflows/main.yml.github/workflows/test-cmake.ymlsrc/syntax/scan.cpptests/unit/syntax/scan_tests.cpp
💤 Files with no reviewable changes (1)
- .github/workflows/main.yml
The clang driver (createInvocation) doesn't work reliably on Windows with a pure InMemoryFileSystem because it tries to resolve system paths and tools. Using CreateFromArgs (arguments_from_database=true) bypasses the driver entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/syntax/scan_tests.cpp (1)
10-18: Consider making the map parameter const.The helper function only reads from the map but takes a non-const reference. Using
const llvm::StringMap<T>&would improve const-correctness and allow passing const maps.♻️ Suggested improvement
template <typename T> -auto find_by_substr(llvm::StringMap<T>& map, llvm::StringRef substr) { +auto find_by_substr(const llvm::StringMap<T>& map, llvm::StringRef substr) { for(auto it = map.begin(); it != map.end(); ++it) { if(it->first().contains(substr)) { return it; } } return map.end(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/scan_tests.cpp` around lines 10 - 18, Change the parameter of find_by_substr to take a const reference (const llvm::StringMap<T>& map) so the helper is const-correct; update the loop to use the map's const iterator (e.g., auto it = map.begin() will deduce const_iterator when map is const) and keep returning map.end() so the return type matches the const iterators; ensure the llvm::StringRef substr parameter and the contains call remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/syntax/scan_tests.cpp`:
- Around line 10-18: Change the parameter of find_by_substr to take a const
reference (const llvm::StringMap<T>& map) so the helper is const-correct; update
the loop to use the map's const iterator (e.g., auto it = map.begin() will
deduce const_iterator when map is const) and keep returning map.end() so the
return type matches the const iterators; ensure the llvm::StringRef substr
parameter and the contains call remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fae79240-a03c-441a-84d8-4a611432dc1b
📒 Files selected for processing (1)
tests/unit/syntax/scan_tests.cpp
On Windows, paths like `/test/main.cpp` are not truly absolute (no drive letter), so InMemoryFileSystem::makeAbsolute() mangles them. Follow clangd's pattern: add test_root()/test_path() helpers that return `C:\clice-test\...` on Windows and `/clice-test/...` on Unix, and set CWD on InMemoryFileSystem before adding files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/syntax/scan_tests.cpp (1)
346-350: Consider usingcontains()orends_with()for string matching.The assertion uses
find()withstd::string::nposcomparison. Ifpathisllvm::StringRef, itscontains()method would be cleaner. If resolving to absolute paths,ends_with()might be more robust.♻️ Optional: Use StringRef's contains() if available
- EXPECT_TRUE(result.includes[0].path.find("foo.h") != std::string::npos); + EXPECT_TRUE(llvm::StringRef(result.includes[0].path).contains("foo.h"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/syntax/scan_tests.cpp` around lines 346 - 350, The test uses result.includes[0].path.find("foo.h") != std::string::npos for matching; replace that with a more appropriate string check depending on the type: if path is llvm::StringRef use path.contains("foo.h") or, if paths may be absolute, use path.endswith("foo.h") (or ends_with equivalent) to assert the filename match. Update the assertion in the test in scan_tests.cpp to call the appropriate method on result.includes[0].path and keep the EXPECT_TRUE semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/syntax/scan_tests.cpp`:
- Around line 346-350: The test uses result.includes[0].path.find("foo.h") !=
std::string::npos for matching; replace that with a more appropriate string
check depending on the type: if path is llvm::StringRef use
path.contains("foo.h") or, if paths may be absolute, use path.endswith("foo.h")
(or ends_with equivalent) to assert the filename match. Update the assertion in
the test in scan_tests.cpp to call the appropriate method on
result.includes[0].path and keep the EXPECT_TRUE semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b04e69a-f2de-4b51-a600-1bc215830553
📒 Files selected for processing (2)
tests/unit/syntax/scan_tests.cpptests/unit/test/platform.h
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 17, 2026
This was referenced Mar 25, 2026
This was referenced Apr 4, 2026
3 of 6 tasks
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
syntax/scanmodule with three scanning strategies:scan(): lexer-based scan usingscanSourceForDependencyDirectives, extracts includes, module names, and conditional contextscan_fuzzy(): preprocessor-based scan that strips conditionals and#defines, processes all includes unconditionally. ReturnsStringMap<ScanResult>for recursive multi-file scanning. SupportsSharedScanCachefor cross-file directive cachingscan_precise(): preprocessor-based scan that keeps all directives, evaluates conditionals preciselyIncludeOnlyVFSwith clang's nativeDependencyDirectivesGetter+scanSourceForDependencyDirectivesAPIIncludeInfostruct withconditionalandnot_foundflagscompute_preamble_bound(s)fromcompile/preambletosyntax/scanPCHInfo,ModuleInfo,PCMInfointocompile/compilation.h; remove old preamble/module headerssrc/server/) pending redesignCI Changes
git diffcheck in format workflowTest plan
scan(),scan_fuzzy(),scan_precise(), andcompute_preamble_bound(s)Summary by CodeRabbit
Breaking Changes
New Features
Refactor
Tests
Chores