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

Sat Apr 8 23:23:33 PDT 2023

nridge marked an inline comment as done.
nridge added inline comments.

================
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
----------------
hokein wrote:
> 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. 
I think it might make sense to keep this support around for a while, so that users whose clients do not yet support this extension still have //some// indication of which preprocessor branches are inactive.

However, we could make it optional, since some users (for example those who have to work on large sections of code in inactive preprocessor branches) may prefer to see the client-side colors over having it all highlighted as one color.

================
Comment at: clang-tools-extra/clangd/ClangdServer.h:91
+    virtual void onInactiveRegionsReady(PathRef File,
+                                        std::vector<Range> InactiveRegions) {}
   };
----------------
hokein wrote:
> 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`).
Good idea -- added!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143974