[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, the change LG now. Only nits from my side!



================
Comment at: clangd/Compiler.h:19
 #include "../clang-tidy/ClangTidyOptions.h"
+#include "GlobalCompilationDatabase.h"
 #include "index/Index.h"
----------------
NIT: this looks unrelated to the actual change. Maybe revert?


================
Comment at: clangd/TUScheduler.cpp:224
 
+  std::shared_ptr<const ParseInputs> getCurrentFileInputs() const;
+
----------------
Could you add a comment that this is private because `Inputs.FS` is not thread-safe and the client code should take care to not expose it via a public interface.


================
Comment at: clangd/TUScheduler.cpp:255
   mutable std::mutex Mutex;
+  /// Inputs, corresponding to the current state of AST.
+  std::shared_ptr<const ParseInputs> FileInputs;         /* GUARDED_BY(Mutex) */
----------------
This comment is not true anymore, the `FileInputs` might be out-of-sync with the AST for short spans of time.
Maybe something like:
```
/// File inputs, currently being used by the worker.
/// Inputs are written and read by the worker thread, compile command can also be consumed by clients of ASTWorker.
```


================
Comment at: clangd/TUScheduler.cpp:343
+  auto Inputs = std::make_shared<ParseInputs>();
+  Inputs->CompileCommand = CDB.getFallbackCommand(FileName);
+  FileInputs = std::move(Inputs);
----------------
NIT:  maybe add a comment explaining why only `CompileCommand` is set and not the other fields?
Thinking of something like:
```
// Other fields are never read outside worker thread and the worker thread will initialize them before first use.
```


================
Comment at: clangd/TUScheduler.cpp:360
   auto Task = [=]() mutable {
+    // Get the actual command as `Inputs` contains fallback command.
+    // FIXME: some build systems like Bazel will take time to preparing
----------------
IIUC, The comment does not correspond to the latest version.
s/contains fallback command/does not have a command.


================
Comment at: clangd/TUScheduler.cpp:380
+      std::lock_guard<std::mutex> Lock(Mutex);
+      FileInputs.reset(new ParseInputs(Inputs));
+    }
----------------
NIT: maybe avoid using new?
```FileInputs = std::make_shared<ParseInputs>(Inputs)```

Feel free to keep as is too, I find the proposed style simpler to follow, but you could reasonably disagree.


================
Comment at: unittests/clangd/ClangdTests.cpp:1148
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+  // Identifier-based fallback completion doesn't know about "symbol" scope.
+  EXPECT_THAT(Res.Completions,
----------------
Not strictly related to this patch, but maybe we could add a flag to completion results to indicate if the completion happened via a fallback mode or not?

Would make the test code more straightforward and the tests like these would not rely on a particular implementation of the fallback mode (e.g. I can imagine the fallback mode learning about the scopes later on)


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:1390
   Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   CodeCompleteResult Completions = cantFail(runCodeComplete(
----------------
Could you expand why we need this?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607