Make compilation database search more robust#323
Make compilation database search more robust#323alcxpr wants to merge 4 commits intoclice-io:mainfrom
Conversation
WalkthroughThe default compile_commands search path was changed from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Editor/Client
participant Server as Language Server
participant FS as FileSystem
participant DB as CompileDatabaseLoader
Client->>Server: initialize / open workspace
Server->>Server: check config.project.compile_commands_dirs
alt compile_commands_dirs non-empty
Server->>FS: read each configured directory
FS-->>Server: paths to compile_commands.json
else empty (auto-discovery)
Server->>FS: scan workspace root and immediate subdirs for compile_commands.json
FS-->>Server: discovered compile_commands.json paths
end
Server->>DB: load discovered compile_commands.json files
DB-->>Server: parsed compile commands
Server->>Client: initialization complete (with compile DB loaded)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 0
🧹 Nitpick comments (5)
tests/unit/Compiler/CommandTests.cpp (2)
1-10: Check that filesystem helper headers exposefsandpath::joinhere
AutoDiscoveryIntegrationusesfs::create_directories,fs::remove, andpath::join, but this TU only directly includes<llvm/Support/FileSystem.h>and not"Support/FileSystem.h". Iffs/pathare only brought in via that project header (e.g.,namespace path { ... join(...) }, aliasingllvm::sys), this test may be relying on transitive includes fromCompiler/Command.h/Compiler/Compilation.h.Consider explicitly including
"Support/FileSystem.h"(or whichever header definesfsandpath::join) in this file to avoid fragile transitive dependencies.
239-273: MakeAutoDiscoveryIntegrationmore robust and portableNice integration-style check that
load_compile_databaseparsescompile_commands.jsoncorrectly. Two follow‑ups to consider:
- The hardcoded
"/tmp/clice_autodiscovery_test"is POSIX‑specific and may not work on Windows or constrained environments. Using LLVM’s temp directory helpers (e.g.,llvm::sys::fs::createUniqueDirectorywithsystem_temp_directory) or a project helper would make the test portable.- Cleanup currently happens only on the happy path at the end of the test. Using
llvm::make_scope_exit(already included viaScopeExit.h) tofs::removethe file and directory would keep/tmptidy even if an assertion fails midway.These are test-only, so non‑blocking, but will improve reliability across platforms and runs.
src/Server/Lifecycle.cpp (2)
10-28: Auto-discovery logic looks correct; consider minimal error handling/loggingThe
find_compile_commandsimplementation correctly:
- Checks
${workspace}/compile_commands.jsonfirst.- Iterates immediate subdirectories and looks for
compile_commands.jsonin each, usingpath::joinandfs::directory_iterator.Functionally this satisfies the issue: it will find both
${workspace}/compile_commands.jsonand${workspace}/build/compile_commands.json(and similar).Two small, non‑blocking ideas:
- If
fs::directory_iteratorsetsec(e.g., permission issues), we currently just stop scanning silently. A debug‑level log whenecis set would make diagnosing discovery issues easier.- If multiple
compile_commands.jsonfiles are found, we silently load all. If order matters, documenting that “all discovered DBs are loaded, in directory iteration order” would help future readers.No correctness blockers here.
64-72: Newon_initializebehavior aligns with auto-discovery goalThe split logic:
- When
config.project.compile_commands_dirsis empty, callfind_compile_commands(workspace)andload_compile_databaseon each discovered file.- When it’s non‑empty, preserve previous behavior and load
path::join(dir, "compile_commands.json")for each configured directory.This:
- Keeps explicit
compile_commands_dirsbehavior intact.- Extends default behavior from just
${workspace}/buildto root + all immediate subdirectories, covering./compile_commands.jsonas requested.One minor improvement to consider: if auto‑discovery finds nothing, a warning log (e.g., “no compile_commands.json found under workspace; semantic features may be degraded”) would make misconfiguration more obvious to users.
Otherwise this looks good.
include/Server/Config.h (1)
24-24: Defaultingcompile_commands_dirsto empty is reasonable with auto-discovery in placeChanging the default from
{"${workspace}/build"}to{}is consistent with the new auto‑discovery logic inServer::on_initialize: an empty vector triggers scanning of the workspace root and immediate subdirectories, which still includes${workspace}/buildwhile adding support for./compile_commands.json.You may want to ensure any user‑facing configuration docs mention that:
- Leaving
compile_commands_dirsunset/empty enables auto‑discovery, and- Setting it explicitly disables auto‑discovery in favor of the configured directories.
From a code perspective, the new default is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/Server/Config.h(1 hunks)src/Server/Lifecycle.cpp(2 hunks)tests/unit/Compiler/CommandTests.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Server/Lifecycle.cpp (1)
include/Support/FileSystem.h (1)
path(15-24)
⏰ 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 (windows-2025, RelWithDebInfo, clang, clang++)
- GitHub Check: build (macos-15, debug)
- GitHub Check: build (macos-15, Debug, clang, clang++)
- GitHub Check: build (ubuntu-24.04, Debug, clang-20, clang++-20)
- GitHub Check: build (macos-15, releasedbg)
- GitHub Check: build (ubuntu-24.04, releasedbg)
- GitHub Check: build (ubuntu-24.04, debug)
- GitHub Check: build (windows-2025, releasedbg)
1 participant
Fix #223.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.