[PATCH] D92663: [clangd] Add hot-reload of compile_commands.json and compile_flags.txt

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

this mostly LGTM. there are some changes to the existing behavior; like discovery order and not looking for other plugins under `build/`, but they seem like non-harmful changes to me.

my biggest question is, have we considered updating `CompilationDatabasePlugin` interface to provide a `loadFromBuffer` and `relativeFileName` virtual methods. I think loading logic could've been a lot simpler and more uniform that way.
i think we discussed this option but i don't remember the outcome, sorry :(.



================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:185
+    bool MayCache = load(*TFS.view(/*CWD=*/llvm::None));
+    if (MayCache) {
+      // Use new timestamp, as loading may be slow.
----------------
nit: inline `MayCache`


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:205
+  auto Stat = FS.status(Path);
+  if (!Stat || !Stat->isRegularFile()) {
+    Size = NoFileCached;
----------------
what about symlinks ? i think users usually have `cc.json -> obscure_build_dir/cc.json` as a symlink.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:224
+  if (HasOldData && NewContentHash == ContentHash)
+    // mtime changed but data is the same: avoid rebuilding the CDB.
+    return {LoadResult::FoundSameData, nullptr};
----------------
also update cached mtime ?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:267
+    // Wrapper for {Fixed,JSON}CompilationDatabase::loadFromBuffer.
+    std::unique_ptr<tooling::CompilationDatabase> (*Parser)(
+        PathRef,
----------------
nit: can we have a functor(probably llvm::function_ref) rather than a function pointer?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:322
+    auto Plugin = Entry.instantiate();
+    if (auto CDB = Plugin->loadFromDirectory(Path, Error)) {
+      log("Loaded compilation database from {0} with plugin {1}", Path,
----------------
it is not crucial but we used to try to load these from `Path + "/build"` as well


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:456
     CDBLookupResult Result) const {
+  vlog("Broadcasting compilation database from {0}", Result.PI.SourceRoot);
   assert(Result.CDB && "Trying to broadcast an invalid CDB!");
----------------
why not `log` instead of `vlog`? this shouldn't be too noisy anyway.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:133
+
+  std::chrono::steady_clock::duration RevalidateAfter;
+  std::chrono::steady_clock::duration RevalidateMissingAfter;
----------------
why not directly store `Options` ?


================
Comment at: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:393
+
+auto HasFlag(llvm::StringRef Flag) { return HasFlag(Flag, "dummy.cc"); }
+
----------------
nit: maybe a `MATCHER_P` instead ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92663/new/

https://reviews.llvm.org/D92663