ioeric added a comment.
In D59811#1445701 <https://reviews.llvm.org/D59811#1445701>, @ilya-biryukov
wrote:
> 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)
All very good points. Thanks! I've added an option.
================
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.",
----------------
ilya-biryukov wrote:
> 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);
> ```
> void runWithPreamble(… Callback<InputsAndPreamble> Action, Callback<Inputs>
> FallbackAction);
This is what I started with in the first iteration. The main problem is that we
would need to bind `CB` to both actions; however, `CB` is not
copyable/shareable by design, I think. This seems to leave us with the only
option to handle everything in the same action. I thought this was fine because
an parameter `AllowFallback` seems to be clear as well. I'm open to suggestions
;)
> This way we don't distinguish between the failure to build a preamble and the
> fallback mode.
I am assuming that a fallback for `runWithPreamble` is either compile command
is missing/broken or preamble fails to be built. And in both cases, fallback
mode could be useful. Do you think we should have more distinctions made here?
================
Comment at: clangd/TUScheduler.cpp:186
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+
----------------
ilya-biryukov wrote:
> 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.
Could you elaborate the advantages of keeping them consistent?
They seem to be updated and used relatively independently. Grouping them into
one function seems to add more work to the call sites. And it seems inevitable
that we would some inconsistency between the two.
================
Comment at: clangd/TUScheduler.cpp:371
+ std::lock_guard<std::mutex> Lock(Mutex);
+ this->CompileCommand = Inputs.CompileCommand;
+ }
----------------
ilya-biryukov wrote:
> 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.
> After this point the clients might start getting a new compile command and an
> old preamble.
Did we have guarantee about this before? It seems that we could also have the
similar situation.
> 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.
I added documentation for `getCurrentPreamble`; would it be the right place?
`getPossiblyStalePreamble` seems self-explaining enough. And since compile
command is always fresher than preamble, I think it would be fine without
commenting?
================
Comment at: clangd/TUScheduler.cpp:918
+ // asynchronous mode, as TU update should finish before this is run.
+ if (!It->second->Worker->isFirstPreambleBuilt() && AllowFallback &&
+ PreambleTasks) {
----------------
ilya-biryukov wrote:
> 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.
It sounds like a very small window (~ms?) that we are trying to optimize for.
Given that users type relatively slow and the first preamble usually only
becomes ready for each file once, it seems unlikely to get value out of this
often. And once the preamble is built, the decision is likely to be always no
fallback, so it would be a bit wasteful to run a separate task for this.
================
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.
----------------
ilya-biryukov wrote:
> 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?
We could potentially use compile command to shorten includes for headers from
global index.
The intention here was to share the same action and thus the same input
structure for both modes.
================
Comment at: unittests/clangd/ClangdTests.cpp:1099
+ clangd::CodeCompleteOptions()))
+ .Completions,
+ IsEmpty());
----------------
ilya-biryukov wrote:
> 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.
Good point. I just realized we could use the `Context` that already exists in
the result. We will set it to `Recovery` in fallback mode. Although sema can
also enter recovery mode, they don't seem to conflict much? WDYT?
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
