[PATCH] D85635: [clangd] Compute the inactive code range for semantic highlighting.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Yup, this is a nice improvement, though there are further things we could do.

As discussed offline, we could consider mapping "disabled" to an attribute but we can't really consume that well in VSCode (or any other editor yet) so let's leave it.



================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:238
+        }
+        size_t LineLength = MainCode.substr(*StartOfLine).find('\n');
+        size_t EndOfLine = LineLength == llvm::StringRef::npos
----------------
A little more direct and avoiding the special case: 
```
StringRef LineText = MainCode.drop_front(StartOfLine).take_until([](char C) { return C == '\n'; });
...
{Position{Line, 0}, Position{Line, lspLength(LineText)}}
```


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:242
+                               : *StartOfLine + LineLength;
+        NonConflicting.push_back(
+            {HighlightingKind::InactiveCode,
----------------
this may overlap existing tokens.
The spec is silent on this, it may pose a challenge to clients and makes for a complex model for little benefit. I think we should avoid overlapping tokens.

For now this can actually happen, as we consider `#ifndef FOO` to be disabled if FOO is defined, but also a usage of the macro FOO.
I'd suggest erasing these (collect their indexes and delete them in a second pass).

In future, I think we probably want to reduce the scope of the disabled regions to not include the directive itself, then this can become an assert.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:83
+                     llvm::ArrayRef<HighlightingToken> Tokens,
+                     size_t NextChar = 0) {
   assert(std::is_sorted(
----------------
AIUI the extra complexity here is to accommodate overlapping ranges, which we can likely get rid of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85635