Page MenuHomePhabricator

[clangd] Inactive regions support via dedicated protocol

Authored by nridge on Feb 13 2023, 7:36 PM.

Diff Detail

Event Timeline

nridge created this revision.Feb 13 2023, 7:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 7:36 PM
Herald added a subscriber: arphaman. · View Herald Transcript
nridge requested review of this revision.Feb 13 2023, 7:36 PM

Corresponding client-side support is implemented for vscode in the latest version of

The issue addressed by this patch is attracting a steady stream of user interest (two recent examples are and

A review would be appreciated :)

Thanks, and sorry for late response.

The patch looks good to me in general.


nit: add trailing // clangd extension, probably better to move to below line 585 (since we group all extension there)


nit: just AST.getMacros().SkppedRanges()?


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. I think we can do it in a separated patch.


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).


nit: notificatin => notification

nridge updated this revision to Diff 511957.Apr 8 2023, 11:22 PM
nridge marked 3 inline comments as done.

Address review comments

nridge marked an inline comment as done.Apr 8 2023, 11:23 PM
nridge added inline comments.

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.


Good idea -- added!

hokein accepted this revision.Apr 13 2023, 3:27 AM

Thanks, the implementation looks good.


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).


nit: mention this is a clangd extension as well.

This revision is now accepted and ready to land.Apr 13 2023, 3:27 AM
nridge updated this revision to Diff 513450.Apr 13 2023, 11:51 PM
nridge marked 2 inline comments as done.

address nit


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.

This revision was landed with ongoing or failed builds.Apr 14 2023, 12:13 AM
This revision was automatically updated to reflect the committed changes.

The patch is causing initialize-params.test to fail, fix coming right up.

The patch is causing initialize-params.test to fail, fix coming right up.

Fixed in