[PATCH] D66735: [clangd] Handling text editor/document lifetimes in vscode extension.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

I think there is one more case  -- we need to cleanup the highlighting cache if clangd crashes, the extension will automatically restart clangd up to 5 times if it sees clangd crashes, you can see how filestatus <https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts#L146-L156> handles it.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:177
 
+  // Called when a text document is closed. Removes any highlighting entries for
+  // the text document that was closed.
----------------
I think `remove all highlightings for the file with the given fileUri.` is enough.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:211
 
-  // Applies all the highlightings currently stored for a file with fileUri.
-  protected applyHighlights(fileUri: string) {
+  // Applies all the highlightings currently stored for a file with fileUri. If
+  public applyHighlights(fileUri: string) {
----------------
nit: the trailing `If`? 


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:212
+  // Applies all the highlightings currently stored for a file with fileUri. If
+  public applyHighlights(fileUri: string) {
+    if (!this.files.has(fileUri))
----------------
could you move this method after `highlight`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66735