[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, and sorry for late response.

The patch looks good to me in general.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:590
       {"foldingRangeProvider", true},
+      {"inactiveRegionsProvider", true},
   };
----------------
nit: add trailing `// clangd extension`, probably better to move to below line 585 (since we group all extension there)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:123
+          ServerCallbacks->onInactiveRegionsReady(Path,
+                                                  std::move(InactiveRegions));
+        }
----------------
nit: just `AST.getMacros().SkppedRanges()`?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:977
+      return CB(InpAST.takeError());
+    // Include inactive regions in semantic highlighting tokens only if the
+    // client doesn't support a dedicated protocol for being informed about
----------------
As discussed in the GitHub thread (the experiment of highlighting the inactive regions as comment is considered as a failure),  we should always not include the inactive region in the semantic highlighting, this will also simplify the existing code in semantic highlighting (e.g. https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SemanticHighlighting.cpp#L464-L513). I think we can do it in a separated patch. 


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:91
+    virtual void onInactiveRegionsReady(PathRef File,
+                                        std::vector<Range> InactiveRegions) {}
   };
----------------
it would be nice to have a unittest for this, I think it should not be to hard to add one in `ClangdTests.cpp` (with a customize Callbacks passing to the `ClangdServer`).


================
Comment at: clang-tools-extra/clangd/Protocol.h:521
+  /// Whether the client supports the textDocument/inactiveRegions
+  /// notificatin. This is a clangd extension.
+  bool InactiveRegions = false;
----------------
nit: notificatin => notification


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143974