[PATCH] D67536: [clangd] Inactive regions support as an extension to semantic highlighting

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, mostly good, a few more nits.



================
Comment at: clang-tools-extra/clangd/ParsedAST.h:134
+  // Ranges skipped during preprocessing.
+  std::vector<SourceRange> SkippedRanges;
 };
----------------
nit: it is not used anymore, remove it.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1212
   std::string Tokens;
+  /// Is the line in an inactive preprocessor branch?
+  bool IsInactive = false;
----------------
hokein wrote:
> could you add some documentation describing this is a clangd extension?
could you document that the line-style highlightings can be combined with any token-style highlightings?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:469
+      auto &AddedLine = DiffedLines.back();
+      for (auto Iter = AddedLine.Tokens.begin();
+           Iter != AddedLine.Tokens.end();) {
----------------
nridge wrote:
> hokein wrote:
> > it took me a while to understand this code,
> > 
> > If the NewLine is `IsInactive`, it just contains a single token whose range is [0, 0), can't we just?
> > 
> > ```
> > 
> > if (NewLine.back().Tokens.empty()) continue;
> > 
> > bool InactiveLine = NewLine.back().Tokens.front().Kind == InactiveCode;
> > assert(InactiveLine && NewLine.back().Tokens.size() == 1 && "IncativeCode must have a single token");
> > DiffedLines.back().IsInactive = true;
> > ```
> An inactive line can contain token highlightings as well. For example, we highlight macro references in the condition of an `#ifdef`, and that line is also inactive if the condition is false. Clients can merge the line style (which is typically a background color) with the token styles (typically a foreground color).
> 
> I did expand the comment to explain what the loop is doing more clearly.
thanks, I see. 

I think we can still simplify the code like below, this could improve the code readability, and avoid the comment explaining it.

```
llvm::erase_if(AddedLine.Tokens, [](const Token& T) { return T.Kind == InactiveCode;});
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536