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

Thu Apr 13 03:27:54 PDT 2023

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

Thanks, the implementation looks good.

================
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
----------------
nridge wrote:
> 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.
Rendering the inactive code as comment is the best we can perform per the standard LSP spec, but it provides an suboptimal (possibly confusing) UX experience, we have received some user complains about that.

Since we are doing it via an extension, I think we should just remove the old one in the next release (17).

================
Comment at: clang-tools-extra/clangd/Protocol.h:1743

+/// Parameters for the inactive regions (server-side) push notification.
+struct InactiveRegionsParams {
----------------
nit: mention this is a clangd extension as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143974