[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

@ilya-biryukov, I hope the current patch is not too big for you to review, happy to chat offline if you want (sam and I had a lot of discussions before he is OOO).



================
Comment at: clangd/ClangdLSPServer.cpp:787
+void ClangdLSPServer::onFileUpdated(const FileStatus &FStatus) {
+  notify("window/showMessage", FStatus);
+}
----------------
sammccall wrote:
> sammccall wrote:
> > notifying via `showMessage` looks sensible at first glance as a fallback.
> > Later we may want to expose a richer API guarded by a client capability, and fallback to `showMessage` if the capability isn't advertised by the client.
> > 
> > Can you verify that `showMessage` is a statusbar-type message and not a dialog in VSCode?
> At the moment you've got `FileStatus` seralizing into the wire format of `ShowMessageParams` which seems at least odd.
> 
> We should define and pass `ShowMessageParams` here, and the transformation from `FileStatus` -> `ShowMessageParams` should be explicit (a named function or maybe better: inlined here) as it has a different semantic meaning than just `toJSON`.
> 
> If the plan is to expose the `FileStatus` object but you're not going to do so in this patch, I think it's OK to leave it in `Protocol.h` - though we should review its structure as if we were going to expose it.
On the second thought, solving the LSP is not the **main** goal of this patch, so I drop this part out (will send out a follow-up patch on the LSP part).


================
Comment at: clangd/ClangdServer.cpp:142
                                WantDiagnostics WantDiags) {
+  // FIXME: emit PreparingBuild file status.
   WorkScheduler.update(File,
----------------
sammccall wrote:
> Can you explain how this should be implemented?
> It seems like not being able to track PreparingBuild is the main weakness of putting the FileStatus in TUScheduler.
> 
> If we can't implement it, fine, but this FIXME suggests it just isn't done yet.
Given the current code, we can't implement it, but I do think this status is valuable (especially for some build system that might be slow to prepare the building environment). Rephased the FIXME, and removed this state in the struct definition (to avoid misleading).


================
Comment at: clangd/ClangdServer.h:48
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(const FileStatus &FStatus){};
 };
----------------
sammccall wrote:
> we should have `PathRef File` here too, for consistency.
> 
> It's redundant with `FileStatus::uri` but I think that field is actually a layering violation: DiagnosticsConsumer knows about file paths, not URIs.
> 
> The more I think about this, the more I think we should move `FileStatus` to the `ClangdServer` or `TUScheduler` layer and exclude URI. If we later want to expose the info via LSP we should define a separate struct in Protocol. WDYT?
Done, use `File` in the interface for consistency.

> The more I think about this, the more I think we should move FileStatus to the ClangdServer or TUScheduler layer and exclude URI. If we later want to expose the info via LSP we should define a separate struct in Protocol. WDYT?

That sounds good. The `FileStatus` aims to be the LSP structure in this patch. I agree we should have a dedicate structure which represents the internal status of TU/file.


================
Comment at: clangd/TUScheduler.cpp:427
             FileName);
+        EmitFileStatus(FileStatusKind::Ready, "reuse prebuilt AST;");
         return;
----------------
sammccall wrote:
> This looks like the wrong layer to me. Suppose you have a queue like this:
> 
> `[update] [go to def] [update] [go to def]`
> 
> As implemented, your status is going to be
> `building    ready    building    ready`
> which seems misleading.
> 
> This makes me think:
>  - the "ready"/"idle" state is a property of the worker thread, not any particular action
>  - maybe we need another status (running action) and then a string field to tell us which action it was.
> 
A sensible status for `[update] [go to def]` would be `queued buildingPreamble buildingFile RunningAction(GoToDef) `


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796