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