ilya-biryukov added a comment.
I think this option should be configurable (and off by default) for the
transition period. A few reasons to do so:
- Before we have an actual implementation of fallback completions, the current
behavior (waiting for the first preamble) actually seems like a better
experience than returning empty results.
- Some clients do this kind of fallback on their own (e.g. VSCode), so until we
can provide actually semantically interesting results (anything more than
identifier-based really, e.g. keyword completions) we would be better off
keeping it off.
- We can still test it if it's off by flipping the corresponding flag in the
test code.
- Would make rollout easier (clients can flip the flag back and forth and make
sure it does not break stuff for them)
================
Comment at: clangd/ClangdServer.cpp:197
return CB(llvm::make_error<CancelledError>());
+ if (!IP->Preamble) {
+ vlog("File {0} is not ready for code completion. Enter fallback mode.",
----------------
This way we don't distinguish between the failure to build a preamble and the
fallback mode.
Have you considered introducing a different callback instead to clearly
separate two cases for the clients?
The code handling those would hardly have any similarities anyway, given that
the nature of those two cases is so different.
Would look something like this:
```
/// When \p FallbackAction is not null, it would be called instead of \p Action
in cases when preamble is
/// not yet ready.
void runWithPreamble(… Callback<InputsAndPreamble> Action, Callback<Inputs>
FallbackAction);
```
================
Comment at: clangd/TUScheduler.cpp:186
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+
----------------
Could we put the preamble and compile command into a single function?
Getting them in the lock-step would mean they're always consistent, which is a
good thing.
================
Comment at: clangd/TUScheduler.cpp:264
+ llvm::Optional<tooling::CompileCommand>
+ CompileCommand; /* GUARDED_BY(Mutex) */
+ /// Becomes ready when the first compile command is set.
----------------
NIT: name it `LastCompileCommand` for consistency with the name of
`LastBuiltPreamble`.
================
Comment at: clangd/TUScheduler.cpp:371
+ std::lock_guard<std::mutex> Lock(Mutex);
+ this->CompileCommand = Inputs.CompileCommand;
+ }
----------------
After this point the clients might start getting a new compile command and an
old preamble.
This seems fine (the clients should be ready for this), but let's document it
in the methods that expose a compile command and a preamble.
================
Comment at: clangd/TUScheduler.cpp:918
+ // asynchronous mode, as TU update should finish before this is run.
+ if (!It->second->Worker->isFirstPreambleBuilt() && AllowFallback &&
+ PreambleTasks) {
----------------
It would be better to make a decision on whether to use a fallback mode in the
actual function scheduled on a different thread (i.e. inside the body of
`Task`).
Imagine the preamble is being built right now and would finish before
scheduling this task. In that case we would have a chance to hit the "preamble"
if we make a decision in the body of the handler, but won't have that chance in
the current implementation.
================
Comment at: clangd/TUScheduler.cpp:977
+ if (!Worker->isFirstCompileCommandSet()) {
+ Worker->waitForFirstCompileCommand();
+ }
----------------
NIT: remove braces
================
Comment at: clangd/TUScheduler.h:44
+ // Can be None in fallback mode when neither Command nor Preamble is ready.
+ llvm::Optional<tooling::CompileCommand> Command;
+ // In fallback mode, this can be nullptr when preamble is not ready.
----------------
How would we expect to use the compile command when preamble is not ready?
Maybe go with passing only contents to the fallback action until we actually
have a use for compile command?
================
Comment at: clangd/TUScheduler.h:156
/// (i.e. WantDiagnostics is downgraded to Auto).
- void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD);
+ void update(PathRef File, FileUpdateInputs UpdateInputs, WantDiagnostics WD);
----------------
NIT: keep the parameter named `Inputs`
================
Comment at: unittests/clangd/ClangdTests.cpp:1099
+ clangd::CodeCompleteOptions()))
+ .Completions,
+ IsEmpty());
----------------
Could we provide a flag in the results indicating this was a fallback-mode
completion?
This could be used here in tests and we could later use it in the clients to
show the corresponding UI indicators for users.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59811/new/
https://reviews.llvm.org/D59811
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
