Repository navigation
refactor: incremental update for compilation database and introduce query toolchain - #311
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 16 minutes and 1 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 (31)
WalkthroughRefactors compilation database and toolchain query interfaces, adds an object-pool utility, threads database-sourced arguments into compilation flows, moves Tester implementations to test code, renames logging macros to LOG_* (adds LOG_*_RET), adds temp-file and toolchain helpers, and exposes test/CI defines in build configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant DB as CompilationDatabase
participant Pool as ObjectPool
participant TC as Toolchain
Client->>DB: lookup(file, options, context?)
DB->>Pool: save_string(file / args)
Pool-->>DB: StringID
DB->>DB: assemble JSONItem -> CompilationInfo, link per-file items
DB-->>Client: CompilationContext { directory, arguments }
Client->>TC: query_toolchain(QueryParams)
TC->>TC: driver_family(driver)
alt Clang-family
TC->>TC: query_clang_toolchain(params)
else GCC-family
TC->>TC: query_gcc_toolchain(params)
else MSVC-family
TC->>TC: query_msvc_toolchain(params)
end
TC-->>Client: cc1-like args (vector<const char*>)
sequenceDiagram
participant App as Application
participant Log as Logging
App->>Log: LOG_INFO("msg")
alt logging enabled
Log->>Log: format & emit
else
Log-->>App: no-op
end
App->>Log: LOG_ERROR_RET(retVal, "err")
alt logging enabled
Log->>Log: emit
Log-->>App: return retVal
else
Log-->>App: return retVal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 7
🧹 Nitpick comments (11)
include/Test/Tester.h (1)
6-81: Tester now targets the newCompilationDatabaseAPI correctlyThe switch to including
Compiler/CompilationDatabase.hand usingdatabase.add_command(...)in bothprepareandcompile_with_pchmatches the new test-only interface, and the rest of the flow (buildingCommandOptions, usinglookup(...).arguments, remapping buffers) remains unchanged.If you later start using
CompilationContext::directoryin tests, consider passing a real directory (e.g."."or a workspace path) instead of"fake"to avoid surprises in any future path-sensitive logic.include/Support/ObjectPool.h (1)
1-63: Make the header self-contained with the necessary standard includes
StringSetand the rest of this header rely on several standard-library facilities (std::uint32_t,std::vector,std::memcpy, type traits,std::destroy_at,std::strong_ordering, etc.) that are not directly included here. Depending on transitive includes from LLVM headers can make this header fragile.Consider adding the minimal required standard headers at the top so
ObjectPool.his self-contained:-#pragma once - -#include "llvm/ADT/StringRef.h" -#include "llvm/ADT/DenseMap.h" -#include "llvm/Support/Allocator.h" +#pragma once + +#include <cstdint> +#include <cstring> +#include <memory> +#include <type_traits> +#include <utility> +#include <vector> +#include <compare> + +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/Support/Allocator.h"include/Compiler/CompilationDatabase.h (1)
72-148: Tighten up the public surface: unusedLookupInfoand missing standard includesTwo minor cleanups to consider in this header:
LookupInfomay now be dead API surfaceWith
lookup()returningCompilationContext, the standaloneLookupInfostruct (directory, arguments, include_indices) appears unused on the public API. If it’s no longer referenced internally either, removing it would reduce confusion for callers looking at this header.Add explicit standard includes for the used types
This header uses
std::optional,std::uint8_t,std::uint32_t, and multiplestd::vector<...>but doesn’t include their headers directly. To avoid depending on transitive includes from other headers, consider adding:#pragma once -#include <expected> +#include <expected> +#include <cstdint> +#include <optional> +#include <vector>This keeps
CompilationDatabase.hself-contained for downstream users.src/Server/Lifecycle.cpp (1)
39-41: Initialize compilation database from all configured compile_commands_dirsSwitching to:
for(auto& dir: config.project.compile_commands_dirs) { database.load_compile_database(path::join(dir, "compile_commands.json")); }correctly generalizes initialization to multiple
compile_commands.jsonlocations. If you want to tighten const‑correctness,const auto& dirwould be a tiny polish, but behavior is fine as-is.tests/unit/Compiler/CompilationDatabase.cpp (1)
198-357: Prefer keeping load tests as skipped tests instead of commenting them outThe
LoadAbsoluteUnixStyleandLoadRelativeUnixStyletests are now entirely commented out. This hides useful documentation and makes it harder to resurrect them later.Consider restoring them as real tests guarded by
skip/skip_unless(...)(as you already do elsewhere), or marking them with a TODO so they still compile but are explicitly skipped. That preserves both examples and structure while acknowledging they’re currently unsupported.src/Server/Document.cpp (1)
146-243: PCH invalidation looks correct; clarify ownership ofCompilationContext::argumentsThe new flow:
check_pch_updatenow compares the preamble slice, the compilation arguments, and dependency mtimes before deciding to rebuild, which is a clear correctness improvement.build_pch_taskmovesinfo.argumentsintoparams.argumentsand uses that for the compile, then writes back a freshPCHInfoplus include links into theOpenFile.Two notes:
- Because
build_pch_taskdoesparams.arguments = std::move(info.arguments);,info.argumentsis left in a moved‑from state. That’s fine today (you don’t useinfoafter scheduling/building the PCH), but it’s worth documenting or constraining the API so future callers don’t accidentally reuseinfoafter calling this helper.- If
CompilationContext::argumentsis a pointer-based container, theinfo.arguments != pch.argumentscomparison relies on the underlying storage being interned/pooled consistently; if that invariant ever changes, this equality check may need to be updated to compare by value instead.Overall the logic in this region is sound; just consider making the move semantics and expectations around
info’s post-call state explicit.src/Compiler/Toolchain.cpp (2)
12-139: Internal helpers and driver_family classification look reasonableThe anonymous-namespace helpers (OptTable “thief”,
CompilerFamily, anddriver_family) are structured sensibly:
driver_familynormalizes.exesuffixes and distinguishes MSVC, Clang/ClangCL, GCC, NVCC, Intel, and Zig based on filename heuristics.- The
ErrorKindalias andunexpected(...)helper simplify error construction forparse_query_resultandquery_driver.No immediate correctness issues here; classification can be refined later if new driver patterns arise.
166-299:query_driverpath resolution and family handling look correct overallWithin
query_driver:
- Resolving non-absolute
drivernames throughllvm::sys::findProgramByNameand then checkingexists/can_executeis a solid precondition check.- Using
driver_family(driver)to branch into GCC/Clang vs. NVCC/Intel/Zig/Unknown vs. MSVC/ClangCL is clear, with explicitNotImplementederrors for unsupported families.- The GCC/Clang branch’s use of a temporary file,
ExecuteAndWait, andparse_query_resultmatches the established pattern and correctly cleans up or preserves the output file.- The MSVC/ClangCL branch reuses clang’s toolchain via
get_toolchainto derive system include paths, which is consistent with your prior workaround.Given these helpers, the main gap is in
query_toolchain;query_driveritself looks coherent.src/Compiler/CompilationDatabase.cpp (3)
69-88: DenseMapInfo specializations rely onStringIDbehaving likestd::uint32_tThe
getEmptyKey/getTombstoneKeyimplementations are fine as long asStringIDreally is astd::uint32_t-like value whose sentinel values can never occur in normal IDs. If that ever changes (e.g.,StringIDbecomes a pointer or struct), these specializations will silently become invalid.Consider either:
- Switching to
DenseMapInfo<StringID>instead ofDenseMapInfo<std::uint32_t>, or- Adding a static_assert that
std::is_same_v<StringID, std::uint32_t>(or whatever the real underlying type is).This keeps the sentinels obviously correct if
StringSet::IDchanges in future.To be safe, verify what
StringSet::IDactually is in this project and whetherDenseMapInfo<std::uint32_t>::getEmptyKey()/getTombstoneKey()align with its own DenseMapInfo specializations. If they differ, update these specializations accordingly.Also applies to: 90-109
734-752:get_option_idrelies on a minimal synthetic argv; consider tightening the contractThe use of
InputArgListwith a two‑elementargumentsarray ({buffer.c_str(), "placeholder"}) is a neat trick to feedParseOneArg, and the"placeholder"handling for options ending in=looks reasonable.Two things you might consider to make this more future‑proof:
- Explicitly document (in a comment) that
arguments[0]here is the option being parsed andarguments[1]is a dummy value for options that take an argument, mirroring how clang’s own driver usesParseOneArg.- If you only care about driver options, you might pass the same include/exclude flag mask that the driver uses (e.g.,
CoreOption/CLOptionmasks) so that non‑driver or unsupported options don’t start returning unexpected IDs if the OptTable grows.Not strictly required for correctness now, but it will make this helper less brittle against upstream OptTable changes.
If you want to be extra safe, compare this usage against the current
clang::driver::getDriverOptTable().ParseOneArgcall sites in upstream clang (Driver.cpp / D67163) to ensure theInputArgListconstruction and flag masks align with how it expects to be used.
766-801: Driver query normalization looks good; watch out for real‑path overhead and filtersThe wrapping of
toolchain::query_driverand subsequent normalization of include paths viafs::real_path+ filtering oflib/gcc/lib\\clangentries makes sense for extracting “clean” system include dirs.Two minor considerations:
fs::real_pathon every include path can be relatively expensive ifquery_driveris called frequently. Since you already have atoolchainscache field inImpl, wiring that up to cacheDriverInfoper(driver, maybe-target)would avoid re‑querying and re‑normalizing on everylookup.- The hard‑coded
"lib/gcc"and"lib\\clang"filters are conservative but a bit heuristic. If you ever need to support non‑standard layouts or custom toolchains, you may want to make these patterns configurable or at least collect them in a single helper.No immediate correctness issue here; mostly an eye towards performance and configurability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CMakeLists.txt(1 hunks)include/Compiler/CompilationDatabase.h(2 hunks)include/Server/Indexer.h(1 hunks)include/Server/Server.h(1 hunks)include/Support/ObjectPool.h(1 hunks)include/Test/Tester.h(3 hunks)src/Compiler/Command.cpp(0 hunks)src/Compiler/Compilation.cpp(1 hunks)src/Compiler/CompilationDatabase.cpp(1 hunks)src/Compiler/Driver.h(1 hunks)src/Compiler/Toolchain.cpp(3 hunks)src/Server/Document.cpp(2 hunks)src/Server/Lifecycle.cpp(1 hunks)tests/unit/Compiler/CompilationDatabase.cpp(7 hunks)xmake.lua(1 hunks)
💤 Files with no reviewable changes (1)
- src/Compiler/Command.cpp
🧰 Additional context used
🧬 Code graph analysis (6)
src/Server/Document.cpp (2)
src/Feature/CodeCompletion.cpp (2)
info(379-381)info(383-385)src/Feature/SignatureHelp.cpp (2)
info(175-177)info(179-181)
src/Compiler/CompilationDatabase.cpp (5)
include/Support/ObjectPool.h (3)
getEmptyKey(194-196)getTombstoneKey(198-200)remove(163-173)include/Compiler/CompilationDatabase.h (1)
CompilationDatabase(81-148)include/Support/FileSystem.h (1)
path(14-23)src/Compiler/Driver.h (2)
parse(95-134)driver(41-45)src/Compiler/Toolchain.cpp (4)
query_driver(166-299)query_driver(166-166)unexpected(87-89)unexpected(87-87)
include/Compiler/CompilationDatabase.h (1)
src/Compiler/CompilationDatabase.cpp (16)
load_compile_database(574-660)load_compile_database(574-574)lookup(662-732)lookup(662-664)get_option_id(734-752)get_option_id(734-734)files(754-760)files(754-754)save_string(762-764)save_string(762-762)query_driver(766-801)query_driver(766-767)add_command(805-814)add_command(805-808)add_command(816-824)add_command(816-818)
src/Compiler/Toolchain.cpp (2)
src/Compiler/Driver.h (1)
driver(41-45)include/Support/FileSystem.h (1)
path(14-23)
tests/unit/Compiler/CompilationDatabase.cpp (1)
include/Test/Tester.h (1)
StringRef(11-27)
include/Support/ObjectPool.h (2)
src/Compiler/Driver.h (1)
clice(11-144)src/Compiler/CompilationDatabase.cpp (14)
T(73-75)T(77-79)T(94-96)T(98-100)value(102-104)value(102-102)lhs(43-46)lhs(43-43)lhs(48-50)lhs(48-48)lhs(85-87)lhs(85-85)lhs(106-108)lhs(106-106)
⏰ 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 (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, releasedbg)
🔇 Additional comments (7)
src/Compiler/Compilation.cpp (1)
4-9: Include update to CompilationDatabase header is consistent with the refactorSwitching the include from the old command header to
Compiler/CompilationDatabase.hkeeps this unit aligned with the new compilation database API, without affecting behaviour in this file.CMakeLists.txt (1)
28-36: Propagating test/CI macros viaclice_optionslooks correctDefining
CLICE_ENABLE_TEST=1andCLICE_CI_ENVIRONMENT=1on theclice_optionsINTERFACE target ensures bothclice-coreand its consumers see the right preprocessor flags, which aligns with the new#ifdef CLICE_ENABLE_TESTand CI-guarded code paths.include/Compiler/CompilationDatabase.h (1)
37-71: New incremental and lookup types line up with the implementationThe API reshaping here looks coherent with the new
CompilationDatabase.cpp:
UpdateKind’sUnchanged/Inserted/Deletedtrio matches an incremental view over a set of commands.DriverInfo.system_includesasstd::vector<const char*>now owns its data, which fits thequery_driverimplementation that builds astd::vectorof pooled strings and avoids danglingArrayRefs.UpdateInfocarrying apath_idand opaquecontextpointer gives you a cheap, stable handle to distinguish multiple compilation contexts for the same file.CompilationContext {directory, arguments}maps cleanly to thelookup()implementation that returns a working directory plus a fully-mangled argument vector.From the call sites you provided (e.g.,
Testerand the snippets inCompilationDatabase.cpp), these changes are consistent and should be backwards-safe for non-test code that only relied on the oldLookupInfo::directory/arguments.include/Server/Server.h (1)
7-7: Server now depends on CompilationDatabase instead of CommandIncluding
Compiler/CompilationDatabase.hhere matches the existingCompilationDatabase database;member and the wider refactor, without altering the Server interface. No issues from this change alone.src/Compiler/Driver.h (1)
3-3: Header dependency updated to CompilationDatabaseSwitching this header to include
Compiler/CompilationDatabase.haligns Driver with the new compilation database API. Given no other changes in this TU, this is a safe dependency update.include/Server/Indexer.h (1)
10-10: Indexer correctly switched to CompilationDatabase headerAdding
Compiler/CompilationDatabase.hhere is consistent with theIndexer(CompilationDatabase& ...)constructor andCompilationDatabase& databasemember. No further changes needed in this file.tests/unit/Compiler/CompilationDatabase.cpp (1)
3-196: Tests correctly updated to new CompilationDatabase API and option parsingThe migration here looks coherent:
- Including
Compiler/CompilationDatabase.hand usingCompilationDatabase::get_option_idin GetOptionID exercises the new public helper appropriately.- Replacing
update_command-style calls withdatabase.add_command(directory, file, argv)in DefaultFilters, Reuse, and RemoveAppend matches the new API and keeps the semantics of those tests intact.- The changed ResourceDir expectations (argument count 5 and separate
"-resource-dir", fs::resource_dir) are consistent with splitting the flag and value into distinct arguments.No issues from these changes; they provide good coverage of the new API surface.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Test/Tester.h (1)
87-93: Avoid dereferencing failedfs::createTemporaryFileresult incompile_with_pchWhen
fs::createTemporaryFile("clice", "pch")fails, the code logs the error but still dereferences*pathlater, which is undefined behavior for an errorExpected/std::expected-like object.Consider logging via
LOG_ERRORand returningfalseearly to avoid using an invalid path:- auto path = fs::createTemporaryFile("clice", "pch"); - if(!path) { - llvm::outs() << path.error().message() << "\n"; - } + auto path = fs::createTemporaryFile("clice", "pch"); + if(!path) { + LOG_ERROR("Fail to create temporary PCH file: {}", path.error().message()); + return false; + }
♻️ Duplicate comments (2)
src/Compiler/Command.cpp (2)
360-369: Guard against empty canonical argument vectors.
mangle_commandassumesinfo.argumentsis non-empty and unconditionally callsarguments.front()at line 421. If a malformedCompilationInforeaches this point with an empty arguments array, this will cause undefined behavior in release builds or assert in debug builds.Add a defensive check:
llvm::SmallVector<const char*, 32> arguments; for(auto arg: info.arguments) { arguments.emplace_back(self.strings.get(arg).data()); } + +// Canonical commands are expected to contain at least the driver. +if(arguments.empty()) { + LOG_WARN("Empty canonical command for file: {}", file); + return std::vector<const char*>{}; +}Also applies to: 421-421, 449-449
770-793: Missingload_commandsimplementation.Per the previous review comment,
load_commandsis declared in the header (include/Compiler/CompilationDatabase.h) and called in tests, but no implementation exists here. This will cause linker errors when building withCLICE_ENABLE_TESTenabled.Either implement
CompilationDatabase::load_commandsin this file under the#ifdef CLICE_ENABLE_TESTblock, or remove the declaration from the header if it's no longer needed.
🧹 Nitpick comments (6)
src/Compiler/Compilation.cpp (1)
103-129: Database-backed invocation path looks correct; consider guarding empty-arg misuseThe
arguments_from_databasebranch correctly treats database-provided arguments as[driver, -cc1, ...], passingdrop_front()toCreateFromArgsand usingparams.arguments[0]asArgv0, while the non-database path preserves the priorcreateInvocationbehavior.To make misuse easier to diagnose, you might add a cheap precondition in the database branch, e.g. an
assert(!params.arguments.empty()), before indexingparams.arguments[0].tests/unit/Compiler/Toolchain.cpp (1)
40-64:GCCtoolchain test is a good smoke test; consider adding assertions or gatingThe
"GCC"test currently just callstoolchain::query_toolchainforg++ -xc++ /dev/nulland prints the resulting arguments. That’s fine as a smoke test, but you might optionally:
- Add a simple assertion (e.g., returned vector non-empty when
g++is available), and/or- Gate the test with a platform/availability check so it can be skipped cleanly when
g++is not on PATH.This would turn the test into a slightly stronger regression guard without depending on specific argument shapes.
tests/unit/Compiler/Command.cpp (1)
243-324: Commented-out CDB load tests reduce coverage; consider refitting instead of disablingThe large
LoadAbsoluteUnixStyleandLoadRelativeUnixStyletests are now fully commented out. That effectively drops coverage for JSON compilation database loading and path normalization.If they’re temporarily incompatible with the new APIs, it would be preferable to:
- Either adapt them to the new
CompilationContext/add_command/query_toolchainmodel, or- Keep them as
skip/skip_unless(...) / test(...)blocks instead of commenting them out entirely, so they remain visible and easy to re-enable.include/Support/Logging.h (1)
95-117: Make LOG_ macros statement-safe and robust for zero-arg calls*The new LOG_* macros work functionally, but two small tweaks would make them safer:
Wrap
LOG_MESSAGEin ado { ... } while(0)
Without this, patterns likeif(cond) LOG_INFO("x"); else ...;can misbind theelse. Encapsulating the macro body in a single statement avoids that.Have
LOG_MESSAGE_RETapply__VA_OPT__directly
As written,LOG_MESSAGE_RET(ret, name, fmt, ...)can end up relying on compiler-specific behavior when__VA_ARGS__is empty. Inlining the logging call avoids nested varargs gotchas.For example:
-#define LOG_MESSAGE(name, fmt, ...) \ - if(clice::logging::options.level <= clice::logging::Level::name) { \ - clice::logging::name(fmt __VA_OPT__(, ) __VA_ARGS__); \ - } +// Single-statement macro, safe in if/else contexts. +#define LOG_MESSAGE(name, fmt, ...) \ + do { \ + if(clice::logging::options.level <= clice::logging::Level::name) { \ + clice::logging::name(fmt __VA_OPT__(, ) __VA_ARGS__); \ + } \ + } while(0) -#define LOG_MESSAGE_RET(ret, name, fmt, ...) \ - do { \ - LOG_MESSAGE(name, fmt, __VA_ARGS__); \ - return ret; \ - } while(0); +#define LOG_MESSAGE_RET(ret, name, fmt, ...) \ + do { \ + if(clice::logging::options.level <= clice::logging::Level::name) { \ + clice::logging::name(fmt __VA_OPT__(, ) __VA_ARGS__); \ + } \ + return ret; \ + } while(0)This keeps semantics the same while making usage in control-flow-heavy code more robust.
src/Compiler/Toolchain.cpp (1)
172-225:parse_version_resultworks structurally but logging and lifetime expectations could be clearer
parse_version_resultcorrectly scans a-v-style output buffer for the target triple and the#include <...> search starts here:/End of search list.block, populatingQueryResult.targetandincludes.Two minor issues to consider:
- The error paths currently do
LOG_ERROR(""), which will emit empty log messages. It would be more actionable to include why parsing failed (e.g., “did not find include-search block markers” or “unterminated include-search block”).QueryResultstoresllvm::StringRefs that point into thecontentbuffer passed in. That’s fine as long as callers ensure the underlying string outlives theQueryResult, but it’s worth documenting or enforcing (e.g., by having the caller own the string and only usingQueryResulttransiently during parsing).include/Compiler/Command.h (1)
58-61: Consider type-safe alternative toconst void*for context.Using
const void*for thecontextfield loses type information and is error-prone. Consider using a type-safe handle (e.g.,std::uintptr_twith a strong typedef, or an opaque handle type) to maintain type safety while allowing flexible context identification.Example using a strong typedef:
struct ContextHandle { const void* ptr; explicit ContextHandle(const void* p) : ptr(p) {} bool operator==(const ContextHandle&) const = default; }; // Then use: ContextHandle context;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
bin/clice.cc(3 hunks)include/Compiler/Command.h(3 hunks)include/Compiler/Compilation.h(1 hunks)include/Compiler/Toolchain.h(1 hunks)include/Support/FileSystem.h(1 hunks)include/Support/Logging.h(1 hunks)include/Test/Tester.h(5 hunks)src/AST/Selection.cpp(5 hunks)src/Async/FileSystem.cpp(1 hunks)src/Async/Network.cpp(3 hunks)src/Async/libuv.cpp(1 hunks)src/Compiler/Command.cpp(6 hunks)src/Compiler/Compilation.cpp(3 hunks)src/Compiler/Tidy.cpp(2 hunks)src/Compiler/Toolchain.cpp(2 hunks)src/Feature/Diagnostic.cpp(1 hunks)src/Feature/Formatting.cpp(1 hunks)src/Server/Document.cpp(17 hunks)src/Server/Feature.cpp(2 hunks)src/Server/Indexer.cpp(6 hunks)src/Server/Lifecycle.cpp(3 hunks)src/Server/Server.cpp(3 hunks)tests/unit/Compiler/Command.cpp(5 hunks)tests/unit/Compiler/Toolchain.cpp(1 hunks)tests/unit/Index/USR.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
bin/clice.cc (1)
src/Async/Network.cpp (4)
listen(71-85)listen(71-71)listen(87-108)listen(87-87)
tests/unit/Compiler/Toolchain.cpp (2)
src/Compiler/Toolchain.cpp (4)
driver_family(229-275)driver_family(229-229)query_toolchain(277-363)query_toolchain(277-277)include/Compiler/Toolchain.h (1)
CompilerFamily(9-49)
include/Test/Tester.h (2)
src/Compiler/CompilationUnit.cpp (3)
CompilationUnit(7-18)diagnostics(217-219)diagnostics(217-217)include/Compiler/CompilationUnit.h (1)
CompilationUnit(15-204)
include/Compiler/Toolchain.h (2)
include/Compiler/Command.h (1)
clice(14-145)src/Compiler/Toolchain.cpp (12)
query_toolchain(277-363)query_toolchain(277-277)driver_family(229-275)driver_family(229-229)driver(129-131)query_gcc_toolchain(365-407)query_gcc_toolchain(365-365)query_clang_toolchain(409-456)query_clang_toolchain(409-409)query_msvc_toolchain(458-476)query_msvc_toolchain(458-458)query_nvcc_toolchain(478-478)
src/Server/Document.cpp (1)
include/Support/FileSystem.h (1)
path(14-23)
src/Compiler/Command.cpp (4)
include/Support/ObjectPool.h (3)
getEmptyKey(194-196)getTombstoneKey(198-200)remove(163-173)src/Compiler/Toolchain.cpp (2)
query_toolchain(277-363)query_toolchain(277-277)include/Support/FileSystem.h (5)
path(14-23)string(27-32)string(84-103)string(108-126)string(128-156)src/Compiler/Driver.h (1)
parse(95-134)
src/Compiler/Toolchain.cpp (2)
include/Support/FileSystem.h (1)
path(14-23)src/Compiler/Driver.h (1)
driver(41-45)
include/Compiler/Command.h (2)
src/Compiler/Command.cpp (14)
load_compile_database(571-657)load_compile_database(571-571)lookup(659-736)lookup(659-661)get_option_id(738-756)get_option_id(738-738)files(758-764)files(758-758)save_string(766-768)save_string(766-766)add_command(772-781)add_command(772-775)add_command(783-791)add_command(783-785)src/Compiler/Toolchain.cpp (2)
query_toolchain(277-363)query_toolchain(277-277)
⏰ 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 (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
🔇 Additional comments (44)
src/Async/FileSystem.cpp (1)
125-125: LGTM: Logging macro updated consistently.The rename from
LOGGING_WARNtoLOG_WARNaligns with the project-wide logging refactor.src/Async/libuv.cpp (1)
32-33: LGTM: Logging macros updated consistently.The rename from
LOGGING_WARNtoLOG_WARNaligns with the project-wide logging refactor. Error handling logic remains unchanged.bin/clice.cc (1)
97-97: LGTM: Logging macros updated consistently.All logging macro renames (
LOGGING_FATAL→LOG_FATAL,LOGGING_INFO→LOG_INFO) align with the project-wide logging refactor. Control flow and error handling remain unchanged.Also applies to: 101-101, 115-115, 121-121, 133-133
src/Server/Indexer.cpp (3)
13-13: LGTM: Enables database-sourced arguments for indexing.Setting
arguments_from_database = truecorrectly sources compilation arguments from the database for the indexing workflow.
19-19: LGTM: Logging macros updated consistently.All logging macro renames (
LOGGING_INFO→LOG_INFO,LOGGING_WARN→LOG_WARN) align with the project-wide logging refactor.Also applies to: 29-29, 59-59, 111-113, 121-121, 141-141, 149-149, 158-158, 163-163
33-33: LGTM: Ensures TUIndex is built and returned.The explicit return statement correctly builds and returns the
TUIndexfrom the compilation unit.include/Support/FileSystem.h (1)
53-63: LGTM: Useful wrapper for temporary file creation.The new
createTemporaryFileoverload provides a convenient wrapper around LLVM's function with consistentstd::expected-based error handling.src/Compiler/Command.cpp (5)
10-65: LGTM: Well-designed data structures for compilation database.The new structures (
CompilationInfo,JSONItem,JSONSource) provide a clean abstraction for managing compilation database entries with efficient string deduplication and support for multiple commands per file.
67-111: LGTM: Correct DenseMapInfo specializations.The specializations for
CompilationInfoandJSONItemproperly implement hashing and equality, following LLVM conventions.
145-248: LGTM: Comprehensive command argument processing.The
save_compilation_infomethods properly filter input/output files and handle various argument formats. The allocation logic at lines 222-226 correctly prevents dangling references.
571-657: LGTM: Robust compilation database loading.The
load_compile_databasemethod provides comprehensive error handling and logging for various invalid JSON formats while correctly building the internal representation.
659-736: LGTM: Context-aware compilation lookup.The
lookupmethod correctly implements context-aware lookups with proper fallback handling and toolchain query integration.src/Feature/Formatting.cpp (1)
44-44: LGTM: Logging macro updated consistently.The rename from
LOGGING_INFOtoLOG_INFOaligns with the project-wide logging refactor.include/Compiler/Compilation.h (1)
26-27: LGTM: Useful flag for argument source selection.The new
arguments_from_databaseflag enables flexible switching between database-sourced and other compilation arguments, with a safe default offalse.src/Async/Network.cpp (1)
33-33: LGTM - Logging macro updates are consistent with the project-wide refactor.The changes from
LOGGING_FATALtoLOG_FATALmaintain identical functionality and align with the broader logging API standardization across the codebase.Also applies to: 60-60, 133-133
src/Feature/Diagnostic.cpp (1)
35-37: LGTM - Logging macro updated consistently.The change from
LOGGING_INFOtoLOG_INFOis consistent with the project-wide logging refactor. The formatting adjustment maintains readability.src/Compiler/Tidy.cpp (1)
337-337: LGTM - Logging macro updates are consistent.All three changes from
LOGGING_INFOtoLOG_INFOmaintain identical functionality and align with the project-wide logging API standardization.Also applies to: 341-341, 394-394
src/AST/Selection.cpp (1)
951-954: LGTM - Logging macro updates are consistent.All changes from
LOGGING_DEBUGtoLOG_DEBUGmaintain identical functionality and align with the project-wide logging API standardization. Debug output remains unchanged.Also applies to: 974-977, 990-990, 1121-1121, 1246-1248
src/Server/Feature.cpp (2)
34-34: LGTM - Enables database-driven argument sourcing for code completion.Setting
arguments_from_database = trueenables the compilation unit to source arguments from the pre-populated database, which is consistent with the broader refactoring to support incremental compilation database updates.
92-92: LGTM - Enables database-driven argument sourcing for signature help.Setting
arguments_from_database = trueenables the compilation unit to source arguments from the pre-populated database, which is consistent with the broader refactoring to support incremental compilation database updates.tests/unit/Index/USR.cpp (1)
39-39: LGTM - Logging macro updates are consistent.The changes from
LOGGING_INFOtoLOG_INFOandLOGGING_FATALtoLOG_FATALmaintain identical functionality and align with the project-wide logging API standardization.Also applies to: 64-64
src/Server/Server.cpp (1)
127-127: LGTM - Logging macro updates are consistent.All changes from
LOGGING_FATAL,LOGGING_WARN, andLOGGING_INFOto theirLOG_*equivalents maintain identical functionality and align with the project-wide logging API standardization.Also applies to: 138-138, 155-155, 163-163, 170-170, 173-173, 180-180
src/Server/Lifecycle.cpp (2)
6-8: LGTM - Logging macro updates are consistent.All changes from
LOGGING_INFO,LOGGING_FATAL, andLOGGING_WARNto theirLOG_*equivalents maintain identical functionality and align with the project-wide logging API standardization.Also applies to: 20-20, 25-25, 27-28
39-41: LGTM - Per-directory compile_commands loading.The change from loading a single
compile_commands.jsonfile to iterating over multiple directories aligns with the PR's objective of supporting incremental compilation database updates and context-aware lookups. This allows each project directory to maintain its own compilation database.include/Test/Tester.h (1)
8-8: Tester harness now correctly uses database + toolchain argumentsIncluding
Support/Logging.h, switching todatabase.add_command, settingoptions.query_toolchain = true, and markingparams.arguments_from_database = truealign this test helper with the new compilation/toolchain pipeline that feeds cc1-ready arguments from the database. This keeps Tester behavior consistent with production paths usingCompilationDatabase::lookupandcreate_invocation.Also applies to: 38-47, 76-86
src/Compiler/Compilation.cpp (1)
351-355: ClearingFrontendOpts.OutputFileforcompileavoids false PCH/PCM error handlingUsing
run_clang<clang::SyntaxOnlyAction>with abefore_executehook that clearsinstance.getFrontendOpts().OutputFileensures the later PCH/PCM guard (which treats non-emptyOutputFileand errors as a PCH/PCM failure) doesn’t misfire when database-sourced arguments still contain an-oflag. This is a good correctness fix in the newarguments_from_databaseflow.tests/unit/Compiler/Toolchain.cpp (1)
11-38: Driver-family classification test covers key naming variants wellThe
expect_familyhelper and"Family"test exercise gcc/g++ (including prefixed/suffixed forms), clang/clang++ with versions and.exe, clang-cl variants, MSVCcl.exe, and zig/zig.exe, which aligns nicely with thedriver_familyimplementation’s normalization logic.tests/unit/Compiler/Command.cpp (2)
92-196: Tests updated toadd_commandalign with new CompilationDatabase APISwitching
DefaultFilters,Reuse, andRemoveAppendto useCompilationDatabase::add_command(with string and argv overloads) while keeping the existing expectations on argument stripping, reuse, and remove/append behavior keeps these tests in sync with the new API without changing their intent. The assertions againstprint_argv(...)still exercise the core normalization logic.
198-219:Modulestub andResourceDirexpectations are consistent with new optionsUsing
.query_toolchain = falsein the"Module"stubbed test exercises the non-toolchain path without forcing toolchain resolution, which is appropriate for a placeholder. In"ResourceDir", expecting 5 arguments with"-resource-dir"andfs::resource_dirinjected before"main.cpp"matches the intended semantics ofCommandOptions{.resource_dir = true}with the updated database lookup.src/Server/Document.cpp (3)
11-79: Cache load/save now use structured logging consistentlyReplacing raw
llvm::outs()/error handling withLOG_WARN/LOG_INFOinload_cache_infoandsave_cache_infostandardizes error and success reporting for cache I/O, while keeping early-return behavior unchanged. This is a straightforward, positive cleanup.Also applies to: 81-142
146-244: PCH invalidation and build now correctly key off CompilationContext + database arguments
check_pch_updatenow compares both the preamble prefix andinfo.argumentsagainst the storedPCHInfo, in addition to dependency mtimes. Combined withbuild_pch_tasksettingparams.arguments_from_database = trueand movinginfo.argumentsintoparams.arguments, PCHs are rebuilt whenever command-line context or dependencies change, while using the new cc1-argument flow.Given that
build_pchalwaysco_awaits thebuild_pch_taskbefore returning, passingCompilationContext& infoand moving frominfo.argumentsinside the task is safe and avoids an extra copy.
248-357: Server now uses toolchain queries + database cc1 args for both PCH and AST buildsIn
build_pchandbuild_ast, enablingoptions.query_toolchain = trueand settingparams.arguments_from_database = truebefore callingcompile(...)aligns the server’s compilation path with the newCompilationDatabase/Toolchainpipeline. Logging of PCH reuse, task cancellation, AST build failures (including diagnostics), and success viaLOG_INFO/LOG_WARN/LOG_FATALprovides clearer observability around these long-lived tasks.include/Compiler/Toolchain.h (1)
9-47: New Toolchain public API is cohesive and matches downstream usage
CompilerFamily,QueryParams, and thequery_toolchain/query_{gcc,clang,msvc,nvcc}_toolchaindeclarations form a coherent surface for toolchain resolution. The shape (QueryParams+std::vector<const char*>suitable forCreateFromArgs) lines up well with howCompilationDatabase::query_toolchainandCompilationParams.arguments_from_databaseare used elsewhere.
query_nvcc_toolchainis clearly marked as a FIXME; just ensure callers treat it as not-yet-supported until a real implementation (or a stub returning an empty vector) is provided.src/Compiler/Toolchain.cpp (3)
14-52: Environment handling andexecute_commandwrapper look solidThe non-Windows
envs()helper builds a cached environment vector that strips any existingLANG=entries and appendsLANG=C, which is appropriate for getting stable ASCII output from GCC/Clang while still inheriting the rest of the environment. On Windows, inheriting the parent environment (std::nullopt) is important for MSVC/clang-cl discovery.
execute_commandwrapsllvm::sys::ExecuteAndWaitwith:
- Temporary-file redirection for stdout/stderr,
- Proper cleanup via
llvm::make_scope_exit, and- LOG_ERROR_RET-based error handling for temp-file creation, process failure, and redirected-file reads.
This gives a clean, reusable primitive for the toolchain queries that need to parse compiler output.
Also applies to: 58-113
229-275:driver_familyclassification matches test expectationsThe
driver_familyimplementation, with its layered normalization (basename,.exestripping, numeric/trailing-component stripping) andtry_getheuristics for cl/nvcc/clang/clang-cl/gcc-family/Intel/zig, matches the cases exercised intests/unit/Compiler/Toolchain.cpp(gcc,g++, versioned clang/clang-cl,cl.exe,zig, etc.). This should behave robustly across common toolchain naming variants.
365-407: Per-family toolchain queries are well structured; nvcc remains a stubThe family-specific implementations:
query_gcc_toolchain:
- Uses
execute_command(..., true)with-dumpmachineand-print-search-dirsto derive--target=and--gcc-install-dir=, then feeds those plus the original arguments throughquery_driver.query_clang_toolchain:
- Handles Zig specially by consuming two leading arguments,
- Runs
driver -### -fsyntax-only ...and tokenizes the emitted cc1 line(s) viaTokenizeGNUCommandLine, filtering out-###/-fsyntax-onlyand feeding the rest through the callback.query_msvc_toolchain:
- Adds
--driver-mode=clin front of the original args and delegates toquery_driver.All three produce
std::vector<const char*>results that are suitable forCompilerInvocation::CreateFromArgsand match how the rest of the pipeline expects cc1 arguments.
query_nvcc_toolchainis currently only declared (no definition), which is consistent with theFIXMEin the header. That’s fine as long as no call sites are added yet; when you do implement NVCC support, this structure provides a clear place to plug it in.Also applies to: 409-456, 458-476, 478-479
include/Compiler/Command.h (8)
24-24: LGTM - Consistent naming with toolchain refactor.The field rename from
query_drivertoquery_toolchainaligns with the broader refactoring toward generalized toolchain queries.
64-70: LGTM - Well-structured compilation context.The new
CompilationContextstruct is well-documented and provides a clear representation of compilation environment with directory and arguments.
99-109: LGTM - Well-designed context-aware API.The new
load_compile_databaseandlookupmethods provide a clean API for incremental compilation database updates and context-aware file lookups. The optionalcontextparameter allows identifying specific compilation contexts when a file has multiple configurations.
118-122: Technical debt acknowledged with FIXME comments.The FIXME comments indicate that
files()andsave_string()APIs are candidates for refactoring. While these methods remain functional, consider prioritizing their redesign in a future iteration to improve the API surface.
127-139: LGTM - Appropriate test scaffolding with acknowledged technical debt.The test-only methods under
CLICE_ENABLE_TESTprovide useful scaffolding for unit tests. Theload_commandsmethod is marked with a FIXME for removal, indicating this is temporary technical debt. Consider addressing this in a follow-up if it's no longer needed.
37-41: UpdateKind enum migration verified as complete and correct.All usages of
UpdateKindin the codebase (6 direct calls insrc/Compiler/Command.cpp) correctly use the new enum values (Unchanged,Inserted,Deleted). The structUpdateInfo(line 53 in Command.h) properly incorporates the enum field. No orphaned references or unhandled enum values exist. The implementation is consistent and complete.
48-48: No call sites found to verify ownership change impact.The field
system_includesat line 48 exists only in the struct definition with no assignments, accesses, or usages anywhere in the codebase. The change fromllvm::ArrayRef<const char*>tostd::vector<const char*>cannot be verified against actual usage patterns. However, this change aligns with similar fields in the same file (e.g.,CompilationContext::argumentsandLookupInfo::arguments, both usingstd::vector<const char*>). Verify that this field is correctly populated at runtime and that any external code (bindings, tests, or serialization) properly handles the ownership semantics of vector-owned data.
55-56: Breaking API change properly implemented; no additional updates required.The
filefield replacement withpath_idhas been correctly migrated throughout the codebase. All six UpdateInfo construction sites insrc/Compiler/Command.cpp(lines 302, 318, 325, 331, 340, 348) pass the second parameter asitem->file_path, which is aStringID(aliased tostd::uint32_t). This matches the expectedpath_idparameter type. The string interning mechanism viaStringSetprovides proper uint32_t ID management, and no legacy.filefield accesses remain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cmake.yml (1)
106-108: Extract macOS LLVM toolchain PATH setup into a reusable composite action.The macOS PATH export duplicates not only within
.github/workflows/cmake.yml(lines 83 and 107) but also appears identically in.github/workflows/xmake.yml(lines 79, 93) and.github/workflows/package.yml(line 113). This pattern repeats across 4 workflow files with 5+ instances total, making it an ideal candidate for a shared composite action to centralize toolchain path management, improve maintainability, and reduce drift.# Example: .github/actions/setup-macos-llvm/action.yml name: 'Setup macOS LLVM' runs: using: 'composite' steps: - if: runner.os == 'macOS' run: echo "PATH=/opt/homebrew/opt/llvm@20/bin:/opt/homebrew/opt/lld@20/bin:$PATH" >> $GITHUB_ENV shell: bashThen replace each duplication with:
uses: ./.github/actions/setup-macos-llvm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cmake.yml(1 hunks)
⏰ 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). (7)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/Compiler/Command.cpp (1)
770-793: Test-onlyadd_commandimplementations are present, butload_commandsis still missing.The two
add_commandoverloads are correctly implemented under theCLICE_ENABLE_TESTguard. However, past reviews noted thatload_commands(declared in the header at line 148-149) is not implemented here, which will cause a linker error when building tests.Per the past review comment: "
load_commandsis declared ininclude/Compiler/Command.h:148-149and called in tests, but no implementation exists. This will cause an undefined symbol linker error when building tests withCLICE_ENABLE_TESTenabled."Either implement
load_commandsin this file under the#ifdef CLICE_ENABLE_TESTblock, or remove the declaration from the header if it's no longer needed.src/Compiler/Toolchain.cpp (1)
362-369: Fix fallback to use corrected argv.The fallback case passes the stripped
argumentstoquery_driver, butarguments[0]is no longer the driver afterconsume_front()at line 315. This causesquery_driverto receive malformed argv.Use
params_copy.arguments(which contains the absolute driver path) instead:std::vector<const char*> result; - query_driver(arguments, [&](const char* driver, llvm::ArrayRef<const char*> cc1_args) { + query_driver(params_copy.arguments, + [&](const char* driver, llvm::ArrayRef<const char*> cc1_args) { result.emplace_back(params.callback(driver)); for(auto arg: cc1_args) { result.emplace_back(params.callback(arg)); } });include/Compiler/Command.h (2)
135-136: Remove unused member method or implement as wrapper.This member method is declared but not implemented, causing potential linker errors. The codebase uses the free function
toolchain::query_toolchaininstead (insrc/Compiler/Toolchain.cpp).Per the past review: "The member method
CompilationDatabase::query_toolchainis declared but not implemented. The codebase uses the free functiontoolchain::query_toolchainwith a different signature. Either remove the unused member method declaration, or implement it as a wrapper around the free function and clarify the TODO."
146-149: Declaration forload_commandslacks implementation.The
load_commandsmethod is declared here but not implemented insrc/Compiler/Command.cpp, which will cause linker errors when building tests withCLICE_ENABLE_TESTenabled.This issue was also flagged in the review of
src/Compiler/Command.cppat lines 770-793.
🧹 Nitpick comments (3)
include/Test/Tester.h (2)
35-48: Toolchain-query / database wiring looks correct; consider verifyingadd_commandusage
prepare()now:
- Registers the command with
database.add_command("fake", src_path, command);- Enables
options.query_toolchain = true;- Marks
params.arguments_from_database = true;- Sources
params.argumentsfromdatabase.lookup(src_path, options).arguments;This matches the contract that database/toolchain-provided arguments are suitable for
CompilerInvocation::CreateFromArgsand correctly flips the flag that the compiler side expects.One thing worth double-checking (in the implementation of
CompilationDatabase) is that the parameter order foradd_commandmatches this usage ("fake"as directory vs driver vs something else). Ifadd_commandpreviously took the same arguments asupdate_command, you’re likely fine, but it’s worth a quick confirm in the implementation/tests.Would you verify that the
add_commandsignature/semantics match this call site (especially the meaning of the first"fake"argument)?
136-141: AST build failure logging is consistent and helpfulThe final AST build in
compile_with_pch()now mirrors the PCH-build error handling: loggingunit.error()and each diagnostic viaLOG_ERROR. This is consistent with other error paths and should make debugging test failures easier.If you’d like to go one step further, you could also replace the
llvm::outs()call on temporary-file creation failure withLOG_ERRORfor fully unified logging, and early-returnfalseinstead of continuing after a failed temp-file creation.src/Compiler/Command.cpp (1)
420-422: Consider defensive check for empty arguments.While
save_compilation_infoensures arguments contain at least the driver, a defensive assertion would make the invariant explicit and prevent undefined behavior if a malformedCompilationInforeaches this function./// Append driver sperately + assert(!arguments.empty() && "canonical compilation command must contain a driver"); add_string(arguments.front());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
include/Compiler/Command.h(4 hunks)include/Test/Tester.h(5 hunks)src/Compiler/Command.cpp(6 hunks)src/Compiler/Compilation.cpp(3 hunks)src/Compiler/Toolchain.cpp(2 hunks)tests/unit/AST/Selection.cpp(2 hunks)tests/unit/Compiler/Toolchain.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/AST/Selection.cpp
- tests/unit/Compiler/Toolchain.cpp
🧰 Additional context used
🧬 Code graph analysis (5)
src/Compiler/Compilation.cpp (1)
tests/unit/Compiler/Command.cpp (2)
print_argv(15-33)print_argv(15-15)
include/Test/Tester.h (2)
include/Compiler/Toolchain.h (1)
clice(7-49)include/Compiler/Compilation.h (1)
clice(12-60)
src/Compiler/Command.cpp (4)
include/Support/ObjectPool.h (3)
getEmptyKey(194-196)getTombstoneKey(198-200)remove(163-173)src/Compiler/Toolchain.cpp (2)
query_toolchain(286-372)query_toolchain(286-286)include/Support/FileSystem.h (5)
path(14-23)string(27-32)string(84-103)string(108-126)string(128-156)src/Compiler/Driver.h (1)
parse(95-134)
include/Compiler/Command.h (3)
tests/unit/Compiler/Command.cpp (2)
print_argv(15-33)print_argv(15-15)src/Compiler/Command.cpp (12)
load_compile_database(571-657)load_compile_database(571-571)lookup(659-736)lookup(659-661)files(758-764)files(758-758)save_string(766-768)save_string(766-766)add_command(772-781)add_command(772-775)add_command(783-791)add_command(783-785)src/Compiler/Toolchain.cpp (2)
query_toolchain(286-372)query_toolchain(286-286)
src/Compiler/Toolchain.cpp (2)
include/Support/FileSystem.h (1)
path(14-23)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, releasedbg)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
🔇 Additional comments (26)
src/Compiler/Compilation.cpp (2)
13-13: Logging include is appropriateIncluding
Support/Logging.hhere is correct and required for the newLOG_ERROR_RETusage below.
355-359: ClearingOutputFileforSyntaxOnlyActionis a good safeguardPassing a
BeforeExecutelambda that clearsinstance.getFrontendOpts().OutputFilefor the plaincompilepath is a solid way to ensure the later PCH/PCM-error check only triggers for actual PCH/PCM builds, not syntax-only compilations that might inherit anOutputFilefrom their arguments.include/Test/Tester.h (3)
8-8: Logging include is consistent with new LOG_ usage*Including
Support/Logging.hhere is appropriate given the newLOG_ERRORcalls in the compilation helpers below.
76-90: PCH compile path wiring matches new database/toolchain semanticsIn
compile_with_pch()you:
- Initialize
params.diagnostics.- Register the command via
database.add_command("fake", src_path, command);- Set
options.query_toolchain = true;- Mark
params.arguments_from_database = true;- Pull
params.argumentsfromdatabase.lookup(src_path, options).arguments;This is consistent with the main
prepare()path and with the expectation that the compilation flow will treat these args as database/toolchain-provided cc1 arguments.If there are tests around PCH-building with the test
CompilationDatabase, please ensure bothprepare()andcompile_with_pch()call paths are covered so regressions inadd_command/query_toolchainusage are caught automatically.
110-119: Error logging for PCH build failures is clearer nowSwitching from direct output to
LOG_ERRORand additionally logging each diagnostic message improves test failure visibility and keeps logging behavior consistent with the rest of the codebase. This looks good.src/Compiler/Toolchain.cpp (6)
20-45: LGTM: Safe environment variable collection.The implementation correctly avoids dangling references by populating the storage vector completely before creating StringRef views, as noted in the comment.
59-116: LGTM: Solid command execution with proper cleanup.Good use of
make_scope_exitto ensure the temporary file is removed, and appropriate platform-specific environment handling.
238-284: LGTM: Comprehensive driver family detection.The progressive stripping approach (executable suffix → version numbers → component suffixes) correctly handles various compiler naming conventions.
374-416: LGTM: GCC toolchain query correctly extracts target and install directory.The implementation properly queries GCC for its configuration and threads the discovered values into the final query_driver call.
418-465: LGTM: Clang/Zig toolchain query correctly parses -### output.The special handling for Zig's two-argument invocation and the filtering of injected flags ensures clean cc1 arguments are returned.
467-485: LGTM: MSVC toolchain query correctly sets driver mode.The
--driver-mode=clflag ensures Clang operates in MSVC-compatible mode, and the implementation properly delegates toquery_driver.src/Compiler/Command.cpp (10)
14-63: LGTM: Internal types support incremental updates and multi-command files.The
JSONItem::nextchaining mechanism correctly enables multiple compilation commands per file, and excluding it from equality checks ensures proper deduplication.
67-111: LGTM: Standard DenseMapInfo implementations.The hash and equality implementations follow LLVM conventions and correctly incorporate all relevant fields for each type.
204-209: Clarify response file handling behavior.The code logs a warning about supporting "only one response file" but then continues processing. Should response file arguments be skipped entirely, or is partial support intended?
Consider either:
- Skipping response files with
continue;(if unsupported), or- Updating the message to indicate partial/first-only support (if intentional).
231-248: LGTM: Command string tokenization with platform detection.The heuristic to detect Windows vs GNU tokenization based on command content is pragmatic, and the existing FIXME appropriately notes the limitation.
250-266: LGTM: Correct chain insertion for multi-command files.The three-case handling (new entry, null entry, append to chain) correctly maintains the linked list of commands per file.
268-287: LGTM: Standard linked list deletion.The implementation correctly handles both head removal and mid-chain unlinking.
289-358: LGTM: Correct incremental merge algorithm.The three-way merge correctly identifies unchanged, inserted, and deleted items, enabling efficient incremental updates to the compilation database.
517-563: LGTM: Comprehensive option filtering for frontend usage.The filtered options correctly exclude compilation-only flags (linking, output, PCH, dependency scanning, modules) that are not needed for frontend analysis.
571-657: LGTM: Robust JSON parsing with proper error handling.The implementation validates all required fields, provides clear error messages for each failure case, and correctly handles both
argumentsarray andcommandstring formats.
738-768: LGTM: Utility functions are straightforward.The helper methods for option lookup, file listing, and string persistence are correctly implemented.
include/Compiler/Command.h (5)
16-35: LGTM: CommandOptions field rename improves clarity.Renaming
query_drivertoquery_toolchainaligns with the updated terminology in the codebase.
37-62: LGTM: Improved naming and added context field.The renamed enumerators (Unchanged, Inserted, Deleted) are more intuitive, and the
contextpointer enables callers to distinguish between multiple compilation commands for the same file.
64-70: LGTM: CompilationContext provides clear API contract.The new struct cleanly encapsulates the working directory and argument list needed for compilation.
81-90: LGTM: Simple debug helper appropriately inlined.The inline
print_argvfunction is lightweight and suitable for header placement.
110-124: LGTM: Public API surface is well-designed.The
lookupsignature with optionalcontextparameter enables both simple and advanced use cases, and the TODO forfetch_alldocuments planned functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Compiler/Compilation.cpp (2)
13-136: Empty-argument guard and invocation branching are correct; consider tightening the log messageThe new
params.arguments.empty()guard plus the database/non-database split remove the UB risk arounddrop_front()andparams.arguments[0], and theLOG_ERROR_RET(nullptr, …)early-returns make the rest of the function safe. This is a solid fix.One small nit: the log text
"empty argument list from database"is emitted even whenarguments_from_database == false, which can be misleading. You also diverge from the more structured"arguments list is: {}"pattern used in the later failure paths.If you want to keep logs consistent and source-agnostic, you could simplify the message like this:
- if(params.arguments.empty()) { - LOG_ERROR_RET(nullptr, "Fail to create invocation: empty argument list from database"); - } + if(params.arguments.empty()) { + LOG_ERROR_RET(nullptr, + "Fail to create invocation, arguments list is: {}", + print_argv(params.arguments)); + }
360-363: ClearingOutputFilefor syntax-only compile is good; consider mirroring for completionMaking
compile(CompilationParams&)explicitly clearinstance.getFrontendOpts().OutputFileensures that syntax-only compilations never accidentally inherit an output path from the underlying command (which would otherwise trip the later “PCH/PCM build” error check). This is a good and targeted change.For consistency and to avoid any accidental coupling to
OutputFilein the code-completion path, consider also clearing it in thecomplete(...)BeforeExecute lambda:- return run_clang<clang::SyntaxOnlyAction>(params, [&](clang::CompilerInstance& instance) { - /// Set options to run code completion. - instance.getFrontendOpts().CodeCompletionAt.FileName = std::move(file); + return run_clang<clang::SyntaxOnlyAction>(params, [&](clang::CompilerInstance& instance) { + /// Make sure the output file is empty for completion, too. + instance.getFrontendOpts().OutputFile.clear(); + + /// Set options to run code completion. + instance.getFrontendOpts().CodeCompletionAt.FileName = std::move(file); instance.getFrontendOpts().CodeCompletionAt.Line = line; instance.getFrontendOpts().CodeCompletionAt.Column = column; instance.setCodeCompletionConsumer(consumer); });Not strictly required, but it keeps all
SyntaxOnlyActioncallers aligned in how they treatOutputFile.src/Compiler/Toolchain.cpp (3)
121-125: Consider whether diagnostic collection is needed for production use.The FIXME at line 121 indicates that diagnostics are currently being ignored via
IgnoringDiagConsumer. This means any compiler warnings or errors during driver query are silently dropped.If diagnostic information would be useful for debugging toolchain query failures, consider implementing diagnostic collection. Otherwise, if this is intentional behavior, update the comment to clarify why diagnostics are not needed here.
186-234: Dead code:parse_version_resultis defined but never used.The function is marked with a TODO indicating future use for parsing
-voutput, but it's currently not called anywhere in the codebase. Dead code increases maintenance burden and can become stale.Options:
- If this is truly needed soon, consider implementing the feature now or opening an issue to track it.
- If not needed in the near term, remove it and re-add when needed.
- If it's being used in code not visible in this PR, please clarify.
Note: The descriptive error messages at lines 226 and 231 appropriately address the past review comment about empty LOG_ERROR calls.
440-463: Consider moving BumpPtrAllocator outside the loop for efficiency.The current implementation creates a new
BumpPtrAllocatorfor each line (lines 448-450). While functionally correct, this is less efficient than creating it once outside the loop.Apply this refactor to improve performance:
std::vector<const char*> result; if(auto content = execute_command(query_arguments, false)) { llvm::SmallVector<llvm::StringRef> lines; llvm::StringRef(*content).split(lines, '\n', -1, /*KeepEmpty=*/false); + llvm::BumpPtrAllocator allocator; + llvm::StringSaver saver(allocator); + for(llvm::StringRef line: lines) { line = line.trim(); if(line.empty() || line.front() != '"') { continue; } llvm::SmallVector<const char*, 256> args; - llvm::BumpPtrAllocator allocator; - llvm::StringSaver saver(allocator); llvm::cl::TokenizeGNUCommandLine(line, saver, args);This is a minor optimization since toolchain queries are not hot-path code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/Compiler/Command.h(4 hunks)include/Support/ObjectPool.h(1 hunks)src/Compiler/Compilation.cpp(3 hunks)src/Compiler/Toolchain.cpp(2 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 (4)
src/Compiler/Compilation.cpp (1)
tests/unit/Compiler/Command.cpp (2)
print_argv(15-33)print_argv(15-15)
include/Support/ObjectPool.h (1)
src/Compiler/Command.cpp (14)
T(73-75)T(77-79)T(94-96)T(98-100)value(102-104)value(102-102)lhs(43-46)lhs(43-43)lhs(48-50)lhs(48-48)lhs(85-87)lhs(85-85)lhs(106-108)lhs(106-106)
include/Compiler/Command.h (2)
tests/unit/Compiler/Command.cpp (2)
print_argv(15-33)print_argv(15-15)src/Compiler/Command.cpp (14)
load_compile_database(571-657)load_compile_database(571-571)lookup(659-736)lookup(659-661)get_option_id(738-756)get_option_id(738-738)files(758-764)files(758-758)save_string(766-768)save_string(766-766)add_command(772-781)add_command(772-775)add_command(783-791)add_command(783-785)
src/Compiler/Toolchain.cpp (2)
tests/unit/Compiler/Command.cpp (2)
print_argv(15-33)print_argv(15-15)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, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, Debug, clang, clang++)
🔇 Additional comments (16)
include/Support/ObjectPool.h (4)
9-63: LGTM! StringSet implementation is sound.The StringSet correctly:
- Reserves ID(0) for the empty string (line 14).
- Interns non-empty strings with a null terminator for C-string compatibility (lines 38-40).
- Uses a cache to avoid duplicate allocations.
65-88: LGTM! The uninitialized pointer issue has been resolved.The in-class initializer
T* ptr = nullptr;(line 67) ensures that default-constructedobject_ptrinstances are safe to inspect or dereference-check without UB.
93-185: LGTM! The memory leak issue has been resolved.The destructor now correctly destroys both active objects (lines 110-114) and removed-but-never-reused objects (lines 116-120), ensuring no resource leaks for non-trivially-destructible types.
189-222: LGTM! DenseMapInfo specialization is correct.The specialization correctly:
- Hashes and compares object values (not pointer identities) at lines 205 and 220.
- Handles empty and tombstone keys (lines 213-218).
include/Compiler/Command.h (5)
16-35: LGTM! Renamingquery_drivertoquery_toolchainaligns with the updated API.The field name now matches the function
toolchain::query_toolchainused in the implementation (seesrc/Compiler/Command.cppline 710).
37-41: LGTM! Enum value renaming improves clarity.The new names
Unchanged,Inserted, andDeletedare more explicit and align with typical CRUD semantics.
51-70: LGTM! UpdateInfo and CompilationContext changes align with the object pool refactor.
path_iduses the string pool ID pattern fromObjectPool.h.contextfield enables tracking multiple compilation contexts per file.CompilationContextcleanly bundles directory and arguments.
110-147: LGTM! New public API methods are implemented and appropriately gated.
load_compile_databaseandlookupare implemented insrc/Compiler/Command.cpp(lines 570-735).- Test-only methods are correctly gated with
CLICE_ENABLE_TEST.- FIXME comments (lines 130, 133) are developer-acknowledged design notes—acceptable for incremental refactoring.
43-49: The original review comment is based on a false premise and should be disregarded.Verification across the entire codebase shows that
DriverInfohas zero usages—no instantiations, field accesses, function parameters, or assignments exist anywhere. Since there are no call sites, the concern about "updating all call sites" is not applicable. The struct appears to be newly added and currently unused, so the ownership change fromArrayReftovectorpresents no breaking changes to existing code.Likely an incorrect or invalid review comment.
src/Compiler/Toolchain.cpp (7)
15-53: LGTM: Cross-platform environment handling is sound.The Unix environment setup correctly filters out
LANGand addsLANG=Cto ensure compiler output is in a parseable format. The static initialization pattern is safe as noted in the comment, and the platform-specificnull_devdistinction is appropriate.
95-108: Address or clarify the FIXME for positive return codes.The FIXME comment at line 102 suggests incomplete error handling. Positive return codes typically indicate the process exited with an error status (e.g., compilation failure), while negative values indicate signals (e.g., crash). Currently all non-zero return codes are treated uniformly.
Consider whether different return code ranges require different handling - for example, compiler errors (rc > 0) might be expected in some scenarios, while crashes (rc < 0) always indicate a problem.
If the current uniform handling is intentional, please remove the FIXME comment and add a clarifying comment explaining why all non-zero codes are treated the same.
238-284: LGTM: Compiler family detection logic is comprehensive.The progressive stripping strategy (executable suffix → version numbers → trailing components) provides robust detection across various compiler naming conventions. The pattern matching covers all supported compiler families appropriately.
286-373: LGTM: Orchestration logic is correct and past issues are addressed.The code correctly addresses the past review comment by using
params_copy.argumentsin the fallback branch (line 363), ensuring the driver is properly included in argv[0].The lifetime management is sound:
pathandsrc_pathSmallStrings remain in scope throughout the family-specific query function calls- Pointers added to
modified_argumentsremain valid for the duration of the function- RAII cleanup pattern for temporary files is appropriate
Minor consistency note: Line 365 uses
params.callbackwhile it could useparams_copy.callbackfor consistency. Both are functionally equivalent sincefunction_refjust copies the reference, but usingparams_copythroughout the modified scope might be slightly clearer.
375-417: LGTM: GCC toolchain query logic is correct.The implementation appropriately:
- Queries target triple via
-dumpmachine- Extracts install directory from
-print-search-dirsoutput- Constructs
--targetand--gcc-install-dirflags for the driver queryThe lifetime of the formatted strings (
target,install_path) is correctly managed - their.c_str()pointers remain valid throughout thequery_drivercall since the SmallStrings are still in scope.
468-486: LGTM: MSVC toolchain query is straightforward and correct.The implementation appropriately adds
--driver-mode=clto invoke the driver in MSVC-compatible mode and delegates toquery_driverfor the actual query logic.
488-488: Incomplete implementation:query_nvcc_toolchainis declared but not defined.The function is declared here but has no implementation. The fallback logic in
query_toolchain(lines 354-372) uses a genericquery_driverapproach for NVCC/Intel/Unknown families, providing basic functionality.This is acceptable if:
- The generic fallback is sufficient for current needs
- There's a tracking issue for implementing full NVCC/Intel support
Otherwise, consider either:
- Implementing the NVCC-specific query logic now
- Opening an issue to track this future work
- Removing the forward declaration if it won't be implemented soon
The TODO comment at line 357 acknowledges this gap, but having a clear plan would be helpful.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/Test/Tester.h(2 hunks)tests/unit/Test/Tester.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/Test/Tester.h
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Test/Tester.cpp (1)
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: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
🔇 Additional comments (2)
tests/unit/Test/Tester.cpp (2)
30-44: LGTM!The compile method has clear control flow with proper error handling and diagnostic logging.
157-163: LGTM!The clear method properly resets all internal state, ensuring the Tester can be reused cleanly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Compiler/Command.cpp (3)
360-514: Duplicate: Guard against empty canonical argument vectors.The code at line 421 calls
arguments.front()and line 449 callsdrop_front()without checking ifargumentsis empty. Whilesave_compilation_infoshould ensure at least the driver is present, defensive coding would prevent undefined behavior if a malformed entry slips through.
704-731: Duplicate: Fragile workarounds in toolchain query integration.The hardcoded insertion at
arguments.begin() + 2(line 714) and the manual-main-file-namerewriting (lines 717-728) are fragile workarounds that should be refactored into the query_toolchain layer. The existing FIXMEs appropriately document this technical debt.
770-793: Duplicate: Missingload_commandsimplementation causes linker errors.The header declares
load_commands(line 116-117 in Command.h) but no implementation exists in this file. This will cause undefined symbol linker errors when building tests withCLICE_ENABLE_TESTenabled. Either implement the method or remove the declaration from the header.
🧹 Nitpick comments (2)
include/Compiler/Command.h (1)
100-104: Documented technical debt in API design.The FIXME comments on
files()andsave_string()indicate known API design issues. Consider tracking these in your issue tracker to ensure they're addressed in future refactoring.src/Compiler/Command.cpp (1)
571-657: LGTM: Robust JSON parsing with comprehensive validation.The implementation properly validates the compilation database format and handles both
argumentsarray andcommandstring formats. Minor typo at line 643:agrsshould beargs(though it's just a local variable name).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Compiler/Command.h(3 hunks)src/Compiler/Command.cpp(6 hunks)tests/unit/Compiler/Toolchain.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Compiler/Toolchain.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/Compiler/Command.cpp (4)
include/Support/ObjectPool.h (3)
getEmptyKey(196-198)getTombstoneKey(200-202)remove(165-175)src/Compiler/Toolchain.cpp (2)
query_toolchain(286-373)query_toolchain(286-286)include/Support/FileSystem.h (5)
path(14-23)string(27-32)string(84-103)string(108-126)string(128-156)src/Compiler/Driver.h (1)
parse(95-134)
include/Compiler/Command.h (2)
src/Compiler/Command.cpp (16)
print_argv(795-806)print_argv(795-795)load_compile_database(571-657)load_compile_database(571-571)lookup(659-736)lookup(659-661)get_option_id(738-756)get_option_id(738-738)files(758-764)files(758-758)save_string(766-768)save_string(766-766)add_command(772-781)add_command(772-775)add_command(783-791)add_command(783-785)include/Support/JSON.h (5)
char(123-127)llvm(165-176)std(62-71)std(137-148)std(151-162)
⏰ 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, Debug, clang, clang++)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, releasedbg)
🔇 Additional comments (15)
include/Compiler/Command.h (6)
24-24: LGTM: Consistent rename toquery_toolchain.The field rename from
query_drivertoquery_toolchainaligns with the broader refactoring in this PR.
37-41: LGTM: Improved enum naming for incremental updates.The rename from
UpdatetoDeletedreflects a clearer incremental update model (Unchanged, Inserted, Deleted). This is a breaking change for existing code using these enum values.
43-54: LGTM: Efficient multi-context support.The change from a string-based
filefield topath_id(uint32_t) improves efficiency through string interning. The addedcontextfield enables tracking multiple compilation contexts for the same file.
56-62: LGTM: Clean compilation context representation.The
CompilationContextstruct provides a clear interface for representing compilation settings with directory and arguments.
64-64: Note: Verify empty-array handling in implementation.Based on past review comments, ensure the implementation guards against empty arrays.
81-95: LGTM: Well-designed public API.The new methods provide a clear interface for loading compilation databases incrementally and looking up compilation contexts. The TODO for
fetch_allappropriately documents future work.src/Compiler/Command.cpp (9)
10-66: LGTM: Well-structured internal types.The internal types (
CompilationInfo,JSONItem,JSONSource) provide a clean model for representing compilation database entries with support for multiple contexts per file through chaining.
67-111: LGTM: Correct DenseMapInfo implementations.The DenseMapInfo specializations follow LLVM conventions correctly with proper empty/tombstone keys, hash functions, and equality checks.
145-229: LGTM with documented limitations.The heuristic approach to removing output files (lines 172-177) is pragmatic given the performance trade-offs. The FIXMEs appropriately document corner cases. However, note that response file support (line 203) currently only logs a warning and skips the file, which may lead to incomplete argument parsing.
231-248: LGTM: Command string tokenization.The implementation correctly handles both Windows and GNU command-line formats. The FIXME at line 240 notes that the detection could be improved.
250-287: LGTM: Correct item chain management.The
insert_itemanddelete_itemmethods properly handle linked-list operations for maintaining multiple compilation contexts per file.
289-358: LGTM: Correct incremental update algorithm.The merge-based incremental update logic correctly identifies Unchanged, Inserted, and Deleted items through a three-way comparison of sorted lists.
517-563: LGTM: Comprehensive option filtering.The constructor properly initializes the filtered options set with all necessary compiler options that should be excluded from the canonical command.
738-756: LGTM: Clean option ID lookup.The implementation correctly uses the Clang driver option table and handles edge cases like arguments ending with
=.
758-768: LGTM: Simple utility methods.Both
files()andsave_string()have straightforward implementations matching their FIXME-documented status in the header.
16bit-ykiko
changed the title
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Compiler/Command.cpp (1)
704-729: Workaround for-main-file-nameis documented but fragile.The FIXME at line 713 and the in-place manipulation of the
-main-file-nameargument (lines 715-726) acknowledge that this logic should ideally be moved into thequery_toolchainlayer. While the current implementation works, the fragile string comparison and array modification make this prone to breakage if toolchain query output formats change.As noted in past reviews, consider centralizing this logic in the query layer when refactoring opportunities arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Compiler/Command.cpp(6 hunks)src/Compiler/Toolchain.cpp(2 hunks)src/Server/Document.cpp(17 hunks)tests/unit/Compiler/Toolchain.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Compiler/Command.cpp (4)
include/Support/ObjectPool.h (3)
getEmptyKey(196-198)getTombstoneKey(200-202)remove(165-175)src/Compiler/Toolchain.cpp (2)
query_toolchain(286-373)query_toolchain(286-286)include/Support/FileSystem.h (5)
path(14-23)string(27-32)string(84-103)string(108-126)string(128-156)src/Compiler/Driver.h (1)
parse(95-134)
src/Compiler/Toolchain.cpp (2)
src/Compiler/Command.cpp (2)
print_argv(793-804)print_argv(793-793)src/Compiler/Driver.h (1)
driver(41-45)
tests/unit/Compiler/Toolchain.cpp (3)
src/Compiler/Toolchain.cpp (4)
driver_family(238-284)driver_family(238-238)query_toolchain(286-373)query_toolchain(286-286)include/Compiler/Toolchain.h (1)
CompilerFamily(9-49)src/Compiler/Compilation.cpp (6)
compile(359-364)compile(359-359)compile(366-393)compile(366-366)compile(395-415)compile(395-395)
⏰ 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, Debug, clang, clang++)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (windows-2025, releasedbg)
- GitHub Check: build (macos-15, debug)
🔇 Additional comments (8)
src/Server/Document.cpp (1)
1-407: LGTM! Clean refactoring to new compilation context API.The changes consistently update logging macros (LOGGING_* → LOG_*), thread the new
CompilationContextthrough PCH/AST workflows, replacequery_driverwithquery_toolchain, and properly set thearguments_from_databaseflag. The refactoring is well-integrated across cache management, PCH building, and AST compilation paths.src/Compiler/Toolchain.cpp (3)
20-45: LGTM! Thread-safe static initialization pattern.The
envs()function correctly uses C++11's thread-safe static initialization. The comment at lines 35-37 explicitly addresses the safety of storingStringRefs after the storage vector is fully populated, preventing dangling references.
59-116: LGTM! Robust command execution with proper cleanup.The
execute_commandfunction correctly manages temporary files via RAII (llvm::make_scope_exit), handles platform-specific environment and redirect differences, and provides comprehensive error handling at each step. Using a temporary file for output capture avoids potential pipe buffer issues.
286-486: LGTM! Well-structured toolchain query dispatcher.The
query_toolchainfunction properly resolves the driver path, creates a temporary source file with the correct extension, and dispatches to family-specific handlers. The fallback case (lines 362-370) correctly usesparams_copy.arguments(which contains the resolved absolute driver path) instead of the strippedargumentsvariable, addressing the past review concern.The per-family query functions (
query_gcc_toolchain,query_clang_toolchain,query_msvc_toolchain) appropriately handle their respective toolchain quirks.tests/unit/Compiler/Toolchain.cpp (1)
1-126: LGTM! Good test coverage for toolchain detection and querying.The test suite effectively validates:
- Driver family detection across multiple compiler variants (lines 21-43)
- End-to-end toolchain query and compilation for GCC and Clang (lines 47-117)
- Proper argument generation including
-cc1presenceThe empty MSVC and Zig test placeholders (lines 81-83, 119-121) are acceptable for incremental development.
src/Compiler/Command.cpp (3)
10-111: LGTM! Well-designed data structures for incremental compilation database.The new types (
CompilationInfo,JSONItem,JSONSource) effectively model the compilation database with support for:
- Canonical command storage via string IDs
- Per-file command chaining
- Incremental update tracking
The
DenseMapInfospecializations are correctly implemented with proper hashing and equality semantics.
289-358: LGTM! Efficient incremental update algorithm.The
update_sourcefunction correctly implements a merge-based incremental update:
- Sorts new items for efficient comparison (line 294-296)
- Uses two-pointer merge to detect insertions, deletions, and unchanged entries (lines 313-352)
- Maintains internal consistency via
insert_item/delete_item- Returns detailed update information for callers
The algorithm is efficient O(n log n) and handles all edge cases correctly.
145-514: LGTM! Solid refactoring with documented technical debt.The
save_compilation_infoandmangle_commandimplementations introduce robust command canonicalization with:
- Heuristic-based output file detection (lines 163-201)
- Proper handling of -I path resolution relative to working directory (lines 479-488)
- PCH flag workarounds for CMake compatibility (lines 491-502)
- Comprehensive argument filtering via the option parser
The FIXMEs (lines 158, 172, 184, 203, 434, 447, 508, 710, 713) appropriately document known limitations and corner cases. The acknowledged heuristics are pragmatic trade-offs between performance and completeness.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.