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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
