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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
