Repository navigation
refactor: CompilationDatabase and scan - #286
Conversation
|
Warning Rate limit exceeded@16bit-ykiko has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughRefactors Token/Lexer naming and const-correctness, replaces Lexer impl pointer with std::unique_ptr and adds advance_if overloads; adds a Scan API for extracting includes and module names; converts CompilationDatabase to a PImpl-based API with lookup/update; adds driver-query/toolchain APIs and updates callsites to lookup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB as CompilationDatabase (PImpl)
participant Impl
Client->>DB: lookup(file, options)
DB->>Impl: resolve or fetch cached command
Impl->>Impl: build LookupInfo {directory, arguments, include_indices}
Impl-->>DB: LookupInfo
DB-->>Client: LookupInfo
sequenceDiagram
participant Source as Source Text
participant Lexer
participant Scanner as scan()
participant Result as ScanResult
Source->>Lexer: construct with content
loop tokens
Lexer->>Scanner: lex token
alt header-name
Scanner->>Result: append Inclusion (angled, line, file)
else preprocessor keyword "module"
Scanner->>Lexer: collect module name tokens until eod
Scanner->>Result: append module_name tokens
end
end
Scanner-->>Result: return ScanResult
sequenceDiagram
participant User
participant query_driver()
participant DriverTool
participant Parser as parse_query_result
User->>query_driver(): invoke(driver)
query_driver()->>DriverTool: run/inspect driver (GCC/Clang/MSVC)
alt GCC/Clang
DriverTool-->>query_driver(): verbose output -> temp file
query_driver->>Parser: parse include paths
else MSVC/ClangCL
query_driver->>DriverTool: use clang toolchain info
else unsupported
query_driver-->>User: return NotFound/NotImplemented error
end
query_driver-->>User: std::expected<QueryResult, QueryDriverError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/AST/SourceCode.h (2)
51-66: LocalSourceRange uses mixed inclusive/exclusive semantics (off‑by‑one risks).text() treats end as exclusive, but contains()/intersects() use end inclusive. Align to half‑open [begin, end) to match substr and token lengths.
Apply:
- constexpr auto length() { - return end - begin; - } + constexpr auto length() const { + assert(end >= begin && "Invalid LocalSourceRange"); + return end - begin; + } @@ - constexpr bool contains(uint32_t offset) const { - return offset >= begin && offset <= end; - } + constexpr bool contains(uint32_t offset) const { + return offset >= begin && offset < end; + } @@ - constexpr bool intersects(const LocalSourceRange& other) const { - return begin <= other.end && end >= other.begin; - } + constexpr bool intersects(const LocalSourceRange& other) const { + return begin < other.end && end > other.begin; + } @@ - constexpr bool valid() const { - return begin != -1 && end != -1; - } + constexpr bool valid() const { + return begin != static_cast<uint32_t>(-1) && + end != static_cast<uint32_t>(-1) && + begin <= end; + }
3-6: Fix header includes; use correct LLVM header for function_ref.The review correctly identifies that this header uses types without including their headers. However, the suggested diff contains an error:
llvm::function_refis inllvm/ADT/STLFunctionalExtras.h, notSTLExtras.h. For LLVM 20.1.5 (your pinned version), use the corrected include path.#pragma once -#include <tuple> +#include <tuple> +#include <optional> +#include <memory> +#include <cstdint> +#include <cassert> +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/STLFunctionalExtras.h" #include "clang/Lex/Token.h" #include "clang/Basic/SourceLocation.h"
🧹 Nitpick comments (11)
include/AST/SourceCode.h (2)
83-86: Make Token::valid() const.Enables checks on const Tokens and matches other const‑correct APIs.
- bool valid() { + bool valid() const { return range.valid(); }
148-150: Docstring doesn’t match signature.Comment says “kind is the param” but overload accepts a predicate. Update to avoid confusion.
- /// Advance the lexer if the next token kind is the param. + /// Advance the lexer if the predicate returns true for the next token.src/AST/SourceCode.cpp (1)
10-12: Nit: spelling in comment.“paring directive” → “parsing directive”.
src/Compiler/Scan.cpp (2)
34-42: Collecting module name tokens works; consider trimming punctuation in API or here.You currently return both identifier(s) and the trailing ':' token. If callers expect a bare module name, either trim punctuation or document this. Optional, just flagging it.
15-19: Optional: compute 1‑based line numbers for includes.
lineis 0. Consider counting newlines up to token.range.begin.Example:
auto line = static_cast<uint32_t>(llvm::count(content.take_front(token.range.begin), '\n') + 1);include/Compiler/Scan.h (6)
3-3: Remove unused<string>include.The header includes
<string>but doesn't usestd::stringanywhere. Onlyllvm::StringRefis used for string handling.-#include <string> #include <vector>
11-11: Fix terminology: "braced angles" → "angle brackets".The correct term for
<>is "angle brackets" (or "angle braces"), not "braced angles".- /// Whether this file is braced angles. + /// Whether this file uses angle brackets (e.g., <iostream> vs "file.h").
17-18: Document lifetime dependency offilefield.The
llvm::StringRefis non-owning and references the originalcontentpassed toscan(). Ifcontentis destroyed before thisInclusionis used, accessingfileresults in undefined behavior. This constraint should be documented.- /// The included file. + /// The included file (valid only while the source content remains alive). llvm::StringRef file;
22-23: Clarify documentation: "module file" → "module name".The comment says "The module file" but the field stores module name tokens, not a file path.
- /// The module file of this file(may be empty). + /// The module name tokens for this file (may be empty). std::vector<Token> module_name;
25-26: Fix grammar in comment."The includes of file" is missing "this".
- /// The includes of file. + /// The includes of this file. std::vector<Inclusion> includes;
29-30: Enhance documentation for the public API.The documentation is too minimal for a public API. Consider documenting:
- What kind of scanning is performed (e.g., "Performs lightweight lexical analysis to extract
#includedirectives and module declarations")- Lifetime constraint: the returned
ScanResultcontains non-owning references tocontentand is invalid aftercontentis destroyed- Behavior with malformed input
-/// Scan the file and return necessary info. +/// Performs lightweight lexical scanning of C++ source code to extract +/// #include directives and module declarations. +/// +/// @param content The source code to scan. +/// @return A ScanResult containing non-owning references to the source. +/// The result is valid only while `content` remains alive. ScanResult scan(llvm::StringRef content);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/AST/SourceCode.h(6 hunks)include/Compiler/Scan.h(1 hunks)src/AST/SourceCode.cpp(3 hunks)src/Compiler/Scan.cpp(1 hunks)src/Feature/SemanticToken.cpp(2 hunks)tests/unit/Compiler/Preamble.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/unit/Compiler/Preamble.cpp (1)
src/Compiler/Scan.cpp (2)
scan(5-46)scan(5-5)
include/Compiler/Scan.h (2)
include/AST/SourceCode.h (1)
clice(40-118)src/Compiler/Scan.cpp (2)
scan(5-46)scan(5-5)
include/AST/SourceCode.h (2)
src/AST/SourceCode.cpp (6)
Lexer(13-24)Lexer(26-26)advance_if(99-107)advance_if(99-99)advance_until(109-116)advance_until(109-109)src/Compiler/Preamble.cpp (1)
lexer(20-20)
src/Feature/SemanticToken.cpp (1)
src/AST/SourceCode.cpp (2)
last(70-72)last(70-70)
src/Compiler/Scan.cpp (2)
src/Compiler/Preamble.cpp (1)
lexer(20-20)src/AST/SourceCode.cpp (2)
next(74-82)next(74-74)
src/AST/SourceCode.cpp (2)
include/AST/SourceCode.h (1)
Lexer(120-190)src/Compiler/Preamble.cpp (1)
lexer(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, release)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: linux (ubuntu-24.04, release)
🔇 Additional comments (2)
src/Feature/SemanticToken.cpp (2)
136-138: Directive detection path update looks good.Switching to token.is_pp_keyword aligns with the new lexer semantics. No issues spotted.
179-181: Macro detection update is consistent.Checking last.is_pp_keyword before "define" matches the new tokenization.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Server/Indexer.cpp (1)
61-83: schedule_next can index out of bounds when no slot is free.If no workings[i].empty() is found, i == workings.size() and co_await workings[i] is UB. Also potential contention among multiple schedulers scanning from 0.
- auto i = 0; - for(; i < workings.size(); i++) { + auto i = 0; + for(; i < workings.size(); ++i) { if(workings[i].empty()) { workings[i] = index(file); break; } } - - co_await workings[i]; - workings[i].release().destroy(); + if (i == workings.size()) { + // No free slot; wait for progress and retry + co_await update_event; + continue; + } + co_await workings[i]; + workings[i].release().destroy();Also applies to: 85-101
src/Server/Feature.cpp (1)
28-35: Options prepared but not used in lookup (resource_dir silently ignored)You create CommandOptions with resource_dir=true but pass none to lookup.
Apply:
- params.arguments = database.lookup(path).arguments; + params.arguments = database.lookup(path, options).arguments;
🧹 Nitpick comments (10)
include/Test/Tester.h (1)
40-46: Defensive check on database.lookup(...) before dereference.Avoid hard failure if lookup fails; return false with diagnostic.
- params.arguments = database.lookup(src_path, options).arguments; + if (auto cmd = database.lookup(src_path, options)) { + params.arguments = cmd->arguments; + } else { + llvm::outs() << "lookup() failed for: " << src_path << "\n"; + return; + }And similarly in compile_with_pch.
Also applies to: 76-82
xmake.lua (1)
59-67: Link set update looks good; watch for GNU ld order sensitivity.With lld you’re fine; if anyone builds with GNU ld, consider wrapping the LLVM/Clang libs in start/end group to avoid order issues.
-- before links = { ... } add_ldflags("-Wl,--start-group") -- after list add_ldflags("-Wl,--end-group")src/Compiler/Driver.h (7)
88-96: Overasserting on parser internals; remove brittle asserts.Asserting dash-dash/grouped-short state couples to LLVM internals. Keep parser resilient regardless of these flags.
- /// Make sure we are not using - assert(!enable_dash_dash_parsing(option_table)); - assert(!enable_grouped_short_options(option_table)); + // ParseOneArg should work regardless of OptTable flags; do not assert on internals.
95-134: parse(): potential invalid asserts on failure paths.The assumptions
it >= arguments.size()andit - prev - 1may not hold for unknown/pass-through options, causing aborts in debug builds.- if(!arg) [[unlikely]] { - assert(it >= arguments.size() && "unexpected parser error!"); - assert(it - prev - 1 && "no missing arguments!"); + if(!arg) [[unlikely]] { + // Let the caller decide how to skip unknown/partial options. on_error(prev, it - prev - 1); break; }
296-301: Prefer explicit nullopt for STDIN/STDOUT; only redirect STDERR to file.Empty-string sentinel for redirects is ambiguous. Use nullopt for inheritance.
- std::optional<llvm::StringRef> redirects[3] = { - {""}, - {""}, - {output_path.str()}, - }; + std::optional<llvm::StringRef> redirects[3] = { + std::nullopt, // stdin + std::nullopt, // stdout + output_path.str()// stderr + };
310-315: Non-Windows env clobbered; inherit env and override locale only.Passing just {"LANG=C"} drops PATH and others. Either pass nullopt (inherit) or build a merged env with LANG/LC_ALL=C.
- llvm::SmallVector<llvm::StringRef> env = {"LANG=C"}; + // TODO: merge LANG/LC_ALL into the current environment instead of clobbering. + std::optional<llvm::ArrayRef<llvm::StringRef>> env = std::nullopt;And document reliance on English output; consider robust parsing without locale dependence.
187-235: Harden parsing of driver output (framework dirs, empty target).
- Strip the suffix " (framework directory)" on Darwin.
- Validate that a non-empty Target was found; return error otherwise.
- if(in_includes_block) { - info.includes.emplace_back(line); - } + if(in_includes_block) { + llvm::StringRef p = line; + p.consume_back(" (framework directory)"); + info.includes.emplace_back(p.str()); + } @@ - return std::expected<void, QueryDriverError>(); + if (info.target.empty()) { + return unexpected(ErrorKind::InvalidOutputFormat, "Target line not found."); + } + return {};
274-341: Temporary-file retention may leak on repeated failures.Keeping the file on every failure aids debugging but can litter temp. Consider a cap or delete-by-default with an env var/flag to keep.
144-147: Make return type of unexpected explicit for readability.Minor style/readability improvement; avoids CTAD surprises across compilers.
-auto unexpected(ErrorKind kind, std::string message) { - return std::unexpected<QueryDriverError>({kind, std::move(message)}); -}; +[[nodiscard]] static inline auto +unexpected(ErrorKind kind, std::string message) + -> std::unexpected<QueryDriverError> { + return std::unexpected<QueryDriverError>({kind, std::move(message)}); +}src/Server/Document.cpp (1)
170-178: Avoid passing LookupInfo by non-const reference into a coroutinebuild_pch_task moves info.arguments, mutating the caller’s object captured by reference across suspension points. Prefer pass-by-value and move at callsite.
Apply:
-async::Task<bool> build_pch_task(LookupInfo& info, +async::Task<bool> build_pch_task(LookupInfo info, std::string cache_dir, std::shared_ptr<OpenFile> open_file, std::string path, std::uint32_t bound, std::string content, std::shared_ptr<std::vector<Diagnostic>> diagnostics) { ... - task = build_pch_task(info, + task = build_pch_task(std::move(info), config.project.cache_dir, open_file, file, bound, std::move(content), open_file->diagnostics);Also applies to: 275-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)cmake/package.cmake(1 hunks)include/Compiler/Command.h(3 hunks)include/Test/Tester.h(2 hunks)src/Compiler/Command.cpp(1 hunks)src/Compiler/Driver.h(1 hunks)src/Compiler/Scan.cpp(1 hunks)src/Server/Document.cpp(4 hunks)src/Server/Feature.cpp(2 hunks)src/Server/Indexer.cpp(2 hunks)tests/unit/Compiler/Command.cpp(6 hunks)xmake.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Compiler/Scan.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
src/Compiler/Command.cpp (1)
src/Compiler/Driver.h (2)
driver(41-45)parse(95-134)
src/Compiler/Driver.h (1)
src/Compiler/Command.cpp (2)
query_driver(686-689)query_driver(686-687)
include/Compiler/Command.h (3)
src/Compiler/Command.cpp (28)
CompilationDatabase(633-654)CompilationDatabase(656-656)CompilationDatabase(660-660)other(658-658)Self(78-97)Self(78-78)Self(99-117)Self(99-100)Self(119-231)Self(119-122)Self(233-253)Self(233-236)Self(255-315)Self(255-256)Self(317-373)Self(317-319)Self(375-527)Self(375-378)Self(529-551)Self(529-530)get_option_id(662-680)get_option_id(662-662)save_string(682-684)save_string(682-682)lookup(716-719)lookup(716-717)files(721-727)files(721-721)src/Compiler/Driver.h (2)
llvm(284-293)clice(11-235)include/Support/Format.h (4)
formatter(10-22)formatter(40-55)formatter(58-69)formatter(73-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (4)
.gitignore (1)
51-53: Clarify or adjust the scope of the performance profiling entries.Line 51 already ignores the entire
tests/unit/Local/directory, making lines 52-53 technically redundant. Additionally, the unscoped entriesperf.dataandflamegraph.svgwill match these filenames anywhere in the repository, not just within the test directory—potentially ignoring similarly-named files in other contexts.For clarity and to avoid unintended scope, consider one of these approaches:
- Option 1 (preferred if these are only used in tests): Remove lines 52-53 entirely, since the parent directory is already ignored.
- Option 2 (if these should be globally ignored): Keep lines 52-53 as-is, but document why they're needed despite the parent directory already being ignored.
- Option 3 (if these should be scoped to tests only): Replace lines 52-53 with
tests/unit/Local/perf.dataandtests/unit/Local/flamegraph.svgto make the intent explicit.cmake/package.cmake (1)
20-33: LLVMTargetParser is present in LLVM 20.1.x; change is safe.LLVMTargetParser was introduced in November 2022 and LLVM 20 ships with it. The codebase enforces LLVM 20.1.x as the sole supported version (cmake/llvm_setup.cmake line 10), so the library is guaranteed to be available. The addition of LLVMTargetParser to the Debug build's explicit library list is correct and poses no risk.
tests/unit/Compiler/Command.cpp (1)
92-101: Tests correctly exercise the new lookup APICoverage looks good across default filters, reuse, remove/append, and resource-dir paths. No changes requested.
Also applies to: 125-146, 148-196, 240-251, 253-344
include/Compiler/Command.h (1)
126-139: Header/API surface looks consistent with implementationNo changes requested. Ensure CompilationDatabase::load_compile_database forwards to Impl as in .cpp fix.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Compiler/Command.cpp (4)
187-197: Response file support is incomplete.The code captures the response file path and index but never expands its contents into the final arguments. Commands relying on
@filearguments will be incomplete, potentially missing critical flags.
407-414: Incorrect SmallString initialization for joined options.The initializer-list construction
{arg.getSpelling(), arg.getValue(0)}is not safe or idiomatic for joining strings.Use string concatenation instead:
case llvm::opt::Option::RenderJoinedStyle: { - llvm::SmallString<256> first = {arg.getSpelling(), arg.getValue(0)}; + llvm::SmallString<256> first = arg.getSpelling(); + first += arg.getValue(0); add_string(first); for(auto value: llvm::ArrayRef(arg.getValues()).drop_front()) { add_string(value); } break; }
460-463: Response file expansion not implemented.This FIXME confirms that response file contents are never parsed and expanded into the argument list, which is consistent with the issue at lines 187-197.
730-732: Critical: Public API method is a no-op.The
load_compile_databasemethod has an empty body and never forwards to the implementation, making it completely non-functional. Callers will get no effect.Apply this fix:
auto CompilationDatabase::load_compile_database(this Self& self, llvm::ArrayRef<std::string> compile_commands_dirs, - llvm::StringRef workspace) -> void {} + llvm::StringRef workspace) -> void { + return self.impl->load_compile_database(compile_commands_dirs, workspace); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Compiler/Command.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Command.cpp (1)
src/Compiler/Driver.h (2)
driver(41-45)parse(95-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: windows (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
🔇 Additional comments (12)
src/Compiler/Command.cpp (12)
10-33: LGTM!The DenseMapInfo specialization for
ArrayRef<const char*>follows LLVM conventions correctly with appropriate empty/tombstone keys and hash implementation.
39-51: LGTM!The CommandInfo struct provides a clean representation of compilation command metadata. The response_file fields are appropriately included for future expansion support.
78-117: LGTM!The string and array caching implementation correctly uses the bump allocator and cache lookups to deduplicate strings and argument lists efficiently.
233-253: LGTM!The driver-based tokenization heuristic appropriately handles Windows (cl/clang-cl) vs. GNU command-line styles. The fallback via filename detection is reasonable for typical use cases.
255-315: LGTM!The JSON parsing correctly handles both
argumentsarray andcommandstring formats per the compilation database specification, with appropriate error handling and path normalization.
317-373: LGTM!The compilation database loading strategy appropriately tries explicit directories first, then falls back to recursive workspace search with sensible filtering of hidden directories.
571-616: LGTM!The lookup method correctly orchestrates command resolution with appropriate fallback, driver querying, and resource directory handling. The implementation properly adds the target file and applies command options.
618-648: LGTM!The command guessing heuristic reasonably searches up to 3 parent directory levels for existing commands with appropriate path separator checks to avoid false matches. The fallback to a sensible default is appropriate.
651-672: LGTM!The constructor appropriately initializes the filtered options set to exclude input files, output options, PCH-related flags, and C++ module flags that should be managed separately.
680-698: LGTM!The
get_option_idimplementation correctly uses the driver option table to parse and identify option IDs, with appropriate placeholder handling for options ending with=.
700-728: LGTM!The public API methods correctly forward calls to the PImpl, maintaining proper encapsulation of the implementation details.
739-745: LGTM!The
files()method correctly extracts and returns all registered file paths from the command info map.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Compiler/Command.cpp (3)
187-197: @response files are recorded but never expanded.You capture
response_fileand its index but never parse/expand its content when building final arguments. Commands relying on @files will be incomplete. The FIXME at line 460 confirms this is a known gap.
407-414: Incorrect SmallString initialization for joined options.Initializer-list construction
{arg.getSpelling(), arg.getValue(0)}is not a safe/idiomatic way to concatenate the spelling and first value.Use concatenation instead:
case llvm::opt::Option::RenderJoinedStyle: { - llvm::SmallString<256> first = {arg.getSpelling(), arg.getValue(0)}; + llvm::SmallString<256> first = arg.getSpelling(); + first += arg.getValue(0); add_string(first); for(auto value: llvm::ArrayRef(arg.getValues()).drop_front()) { add_string(value); } break; }
546-563: Potential use of unresolved path after error.At line 552,
fs::real_pathmay fail and seterr, but line 553 unconditionally assignsinclude = bufferbefore checking the error. While short-circuit evaluation at line 556 prevents using invalid buffer content in thecontains()checks, this pattern is confusing and fragile.Consider checking the error first:
for(llvm::StringRef include: driver_info->includes) { llvm::SmallString<64> buffer; auto err = fs::real_path(include, buffer); - include = buffer; - - /// Remove resource dir of the driver. - if(err || - include.contains("lib/gcc") - /// FIXME: Only for windows, for Mac removing default resource dir - /// may result in unexpected error. Figure out it. - || include.contains("lib\\clang")) { + + /// Skip if real_path failed + if(err) { + continue; + } + + include = buffer; + + /// Remove resource dir of the driver. + if(include.contains("lib/gcc") || include.contains("lib\\clang")) { continue; } includes.emplace_back(self.save_string(include).data()); }src/Compiler/Driver.h (2)
31-51: The Thief pattern remains under discussion from previous reviews.This private member access pattern was already flagged as a major portability concern. Please refer to the existing discussion thread where alternative approaches using
Driver::BuildCompilationwere suggested.
344-367: MSVC/clang-cl path remains under discussion from previous reviews.This code block depends on the
get_toolchain()Thief pattern and was already discussed in previous reviews. The suggestion to useDriver::BuildCompilationwith public APIs is still relevant.
🧹 Nitpick comments (3)
src/Compiler/Driver.h (3)
56-61: Consider throwing an exception instead ofstd::abort().The destructor uses
std::abort()on invariant violations. While this catches programmer errors during development, in production it terminates the process immediately. Consider whether a diagnostic + exception (thrown before the destructor runs) or at least logging would be more appropriate for a library component that might be embedded in a long-running server.
234-234: Simplify the return statement.The explicit construction
std::expected<void, QueryDriverError>()can be simplified.Apply this diff:
- return std::expected<void, QueryDriverError>(); + return {};
282-282: Fix typo."infomation" should be "information".
Apply this diff:
- // If we fail to get the driver infomation, keep the output file for user to debug. + // If we fail to get the driver information, keep the output file for user to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Compiler/Command.cpp(1 hunks)src/Compiler/Driver.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Compiler/Command.cpp (2)
include/Support/FileSystem.h (4)
string(27-32)string(82-101)string(106-124)string(126-154)src/Compiler/Driver.h (2)
driver(41-45)parse(95-134)
src/Compiler/Driver.h (2)
src/Compiler/Command.cpp (2)
query_driver(704-707)query_driver(704-705)include/Support/FileSystem.h (1)
path(14-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos (macos-15, release)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (windows-2025)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
🔇 Additional comments (6)
src/Compiler/Command.cpp (3)
10-33: LGTM! Clean DenseMapInfo specialization.The specialization correctly implements the required interface for using
ArrayRef<const char*>as a DenseMap key, with appropriate sentinel values and hash function.
39-51: LGTM! Well-documented data structure.The
CommandInfostruct clearly represents the canonical command information with appropriate members and documentation.
730-734: LGTM! Previously flagged no-op is now fixed.The public
load_compile_databasemethod now correctly forwards to the PImpl implementation at line 317, resolving the critical issue from the previous review.src/Compiler/Driver.h (3)
88-93: Useparse_oneonly after validation.The method
parse_onecontains assertions that could fire at runtime if the option table is misconfigured. Consider converting these to runtime checks with clear error returns, or document that callers must ensure the option table is properly configured before constructingArgumentParser.
159-180: LGTM—Compiler family detection is well-ordered.The pattern matching correctly checks
clang-clbeforeclangto avoid false positives, and handles Windows.exesuffixes appropriately.
274-340: GCC/Clang driver querying logic is sound.The implementation correctly:
- Creates temporary files for output capture
- Uses platform-specific null device paths (
NULvs/dev/null)- Inherits environment on Windows (needed for MSVC toolchain detection)
- Sets
LANG=Con Unix to ensure ASCII output- Cleans up temporary files with appropriate error logging
16bit-ykiko
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Compiler/Driver.h (2)
13-18: Do not wrap the header API in an anonymous namespace.All of the public declarations (
CompilerFamily,QueryResult,query_driver, etc.) end up with internal linkage because this header still lives inside an anonymous namespace. That makes every translation unit see a different type and function, breaking ODR and preventing callers from linking against a single definition—exactly what was flagged earlier. Please move only the true helpers to an internal namespace and keep the exported API innamespace clicewith external linkage.
31-50: Please retire theThiefhack.The
Thieftemplate continues to poke at private Clang members via pointer-to-member tricks. This relies on LLVM internals that can (and do) change without notice, so it is brittle and will keep breaking with toolchain updates—the same concern raised previously. We still need a solution based on Clang’s public APIs (e.g.,Driver::BuildCompilation, subprocess probing, or similar) instead of private member access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmake/package.cmake(1 hunks)include/Compiler/Toolchain.h(1 hunks)src/Compiler/Driver.h(1 hunks)src/Compiler/Toolchain.cpp(1 hunks)src/Server/Document.cpp(4 hunks)src/Server/Indexer.cpp(2 hunks)xmake.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmake/package.cmake
- src/Server/Indexer.cpp
- xmake.lua
🧰 Additional context used
🧬 Code graph analysis (3)
src/Compiler/Toolchain.cpp (1)
src/Compiler/Driver.h (1)
driver(41-45)
include/Compiler/Toolchain.h (1)
src/Compiler/Toolchain.cpp (2)
query_toolchain(5-11)query_toolchain(5-5)
src/Compiler/Driver.h (3)
include/Compiler/Toolchain.h (1)
clice(6-15)include/Test/Tester.h (2)
clice(9-67)StringRef(11-27)src/Compiler/Command.cpp (3)
it(349-349)query_driver(704-707)query_driver(704-705)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: linux (ubuntu-24.04, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: build (macos-15)
16bit-ykiko
changed the title
CompilationDatabase and scan
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Compiler/Command.cpp (1)
649-678: Remove or comment out unreachable recursive search code.The early
returnat line 649 makes lines 650-678 unreachable. While the FIXME comment at lines 651-652 explains why recursive scanning is disabled, keeping dead code in the source is confusing and violates maintainability best practices.Consider one of these options:
Option 1 (preferred): Remove the dead code entirely
logging::warn( "Can not found any valid CDB file from given directories, search recursively from workspace: {} ...", workspace); - return; - - /// FIXME: Recursive workspace scanning shouldn't be enabled by default, as it - /// might scan unintended directories. - std::error_code ec; - for(fs::recursive_directory_iterator it(workspace, ec), end; it != end && !ec; - it.increment(ec)) { - auto status = it->status(); - if(!status) { - continue; - } - - // Skip hidden directories. - llvm::StringRef filename = path::filename(it->path()); - if(fs::is_directory(*status) && filename.starts_with('.')) { - it.no_push(); - continue; - } - - if(fs::is_regular_file(*status) && filename == "compile_commands.json") { - if(try_load(path::parent_path(it->path()))) { - return; - } - } - } + // TODO: Consider adding opt-in recursive workspace scanning via configuration /// TODO: Add a default command in clice.toml. Or load commands from .clangd ? logging::warn( "Can not found any valid CDB file in current workspace, fallback to default mode."); + return;Option 2: Conditionally enable via a parameter
If you plan to support this feature later, make it conditional:auto CompilationDatabase::load_compile_database(..., bool allow_recursive = false) -> voidand guard the recursive block with
if (allow_recursive).
♻️ Duplicate comments (2)
src/Compiler/Command.cpp (2)
492-502: Response files are recorded but never expanded.The code captures
response_fileand its index but does not parse or expand the file's content when building final arguments inprocess_command. Commands relying on@filesyntax will have incomplete argument lists.Consider:
- Resolve
response_filepath relative toinfo.directoryif not absolute- Read and tokenize the file content using the same driver heuristic (GNU vs Windows) as the command tokenization
- Expand the tokens into the argument list at the appropriate position
As per coding guidelines
151-158: Incorrect SmallString initialization for joined options.Initializer-list construction
{arg.getSpelling(), arg.getValue(0)}is unsafe for concatenating strings.Apply this fix:
case llvm::opt::Option::RenderJoinedStyle: { - llvm::SmallString<256> first = {arg.getSpelling(), arg.getValue(0)}; + llvm::SmallString<256> first = arg.getSpelling(); + first += arg.getValue(0); add_string(first); for(auto value: llvm::ArrayRef(arg.getValues()).drop_front()) { add_string(value); } break; }
🧹 Nitpick comments (1)
src/Compiler/Command.cpp (1)
273-303: Consider making the fallback command configurable.The hardcoded fallback
{"clang++", "-std=c++20"}at line 298 may not suit all projects (e.g., C-only codebases, different C++ standards, custom toolchains). The FIXME at line 295 already notes this.Consider allowing users to specify a default command via configuration (e.g., in
clice.tomlas mentioned in the TODO at line 723), or at minimum, detect the file extension to choose between C and C++ defaults:// Detect language from file extension llvm::StringRef ext = path::extension(file); if(ext == ".c" || ext == ".h") { constexpr const char* fallback[] = {"clang", "-std=c17"}; // ... } else { constexpr const char* fallback[] = {"clang++", "-std=c++20"}; // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Compiler/Command.cpp(12 hunks)xmake.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- xmake.lua
🧰 Additional context used
🧬 Code graph analysis (1)
src/Compiler/Command.cpp (3)
src/Server/Indexer.cpp (4)
index(10-59)index(10-10)lookup(169-215)lookup(169-169)src/Compiler/Toolchain.cpp (4)
query_driver(147-280)query_driver(147-147)unexpected(59-61)unexpected(59-59)src/Compiler/Driver.h (1)
driver(41-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2025)
- GitHub Check: macos (macos-15, debug)
- GitHub Check: linux (ubuntu-24.04, debug)
- GitHub Check: macos (macos-15, release)
- GitHub Check: windows (windows-2025)
- GitHub Check: linux (ubuntu-24.04, release)
🔇 Additional comments (4)
src/Compiler/Command.cpp (4)
10-33: LGTM: DenseMapInfo specialization follows LLVM conventions.The specialization correctly implements the required interface for DenseMap with sentinel pointers, appropriate hashing, and equality comparison.
78-117: LGTM: String and cstring list caching implementation is sound.Both methods correctly implement deduplication and memory pooling with proper null-termination and cache lookups before allocation.
402-425: LGTM: Path resolution error handling is correct.The code properly guards against using unresolved paths by checking the error condition before proceeding with the resolved path, addressing the previously flagged concern.
680-725: LGTM: Well-structured lookup implementation with proper fallbacks.The method provides a clean API with optional driver querying, appropriate error handling, and sensible fallback behavior when no command is found.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores