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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:469
+      auto &AddedLine = DiffedLines.back();
+      for (auto Iter = AddedLine.Tokens.begin();
+           Iter != AddedLine.Tokens.end();) {
----------------
hokein wrote:
> 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;});
> ```
Done. Note that we still need to set `AddedLine.IsInactive` appropriately. I did that in the lambda, which feels like a strange thing for an `erase_if` predicate to do. The alternative would be doing a `count_if` first (but then we're looping over the tokens twice).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67536