[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Leaving a few first comments, have to run.
Will take another look later, sorry about the delay today (also feel free to add more reviewers in case this is urgent).



================
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.",
----------------
ioeric wrote:
> 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?
Ah, right, I did not realize that design of `Callback` makes it hard for two callbacks to own the same data.
And that's what we technically need for this. I'll have to think about this two, maybe there's a better way to express this in the code (`AllowFallback` looks like the option with least amount of friction so far).


================
Comment at: clangd/TUScheduler.cpp:186
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+
----------------
ioeric wrote:
> 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.
I was originally thinking about consistency between the provided compile command and the one used to build the preamble.
Now I realize that they are set at different times, so we can't really make them consistent (unless we decide to delay the update of the compile command, but I don't see why it's useful).

You approach looks legit, but could you please document in these getters (maybe more importantly in the public callback interfaces?) that preamble and compile commands might be out of sync (command can be newer) and clients should handle that gracefully.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811