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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:206
+  auto Stat = FS.status(Path);
+  if (!Stat || !Stat->isRegularFile()) {
+    Size = NoFileCached;
----------------
Here's a fun thought: what if this is a network or otherwise magical filesystem (like a FUSE) and may be temporarily unavailable (due to expiring credentials, network issues, maybe your machine just woke up from suspend and needs to reconnect). 

Do you think it makes sense to check for things like ENOACCES, ENOKEY, ESTALE and treat them as transient?


================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:217
+  if (!Buf || (*Buf)->getBufferSize() != Stat->getSize()) {
+    Size = NoFileCached;
+    elog("Failed to read {0}: {1}", Path,
----------------
Why throw away the information we have? Better keep the cached metadata around, transient error here could mean the build system is re-creating the file, but it may end up being identical to what we already read, it's just that someone re-run cmake or something.


================
Comment at: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:393
+
+auto HasFlag(llvm::StringRef Flag) { return HasFlag(Flag, "dummy.cc"); }
+
----------------
lint warning here. Either rename or silence with a comment


================
Comment at: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:397
+  MockFS FS;
+  auto Stale = std::chrono::steady_clock::time_point::min();
+  auto Fresh = std::chrono::steady_clock::time_point::max();
----------------
Considering you already use min() in code, I would prefer if the test also included a value that's between min() and max() (i.e. something that's not a special-case)


================
Comment at: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:424
+    "directory": "{0}/foo",
+  }])json",
+                                                        testRoot());
----------------
Can you add a couple of calls to test the modtime and size comparison? Like, maybe change DBAZ to DFOO and see that it's picked up despite size being the same if mtime changed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92663