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

[Nathan Ridge via Phabricator via cfe-commits]

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:
> 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).
> I think we should just remove the old one in the next release (17).

To give other clients a bit more time to implement the extension, perhaps we could:

 * in v17: disable the comment tokens by default, but still have an option to turn them on
 * in v18: remove it altogether?

This way, if someone using a non-vscode client has upgraded to clangd 17, but their client hasn't implemented the protocol extension yet, and they prefer to see the comment tokens (to know which branches are inactive), they can turn them on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143974