hokein added a comment. a few more comments from my side.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:309 + // If the New file has fewer lines than the Old file we don't want to send + // highlightings beyond the end of the file. That might crash the client. + for (int Line = 0; Line != std::numeric_limits<int>::max() && Line < NLines; ---------------- nit: I believe theia is crashing because of this? could we file a bug to theia? I think it would be nice for client to be robust on this case. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:70 +// Return a line-by-line diff between two highlightings. +// - if the tokens on a line are the same in both hightlightings this line is +// omitted. ---------------- nit: add `, ` before `this` ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:73 +// - if a line exists in New but not in Old the tokens +// on this line is emitted., we emit the tokens on this line +// - if a line not exists in New but in Old an empty ---------------- could you refine this sentence, I can't parse it. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:74 +// on this line is emitted., we emit the tokens on this line +// - if a line not exists in New but in Old an empty +// line is emitted (to tell client to clear the previous highlightings on this ---------------- and here as well. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80 +diffHighlightings(ArrayRef<HighlightingToken> New, + ArrayRef<HighlightingToken> Old, int NLines); ---------------- ilya-biryukov wrote: > Could you document what `NLines` is? Specifically, whether it's the number of > lines for `New` or `Old`. > could we use a more describe name for `NLines`? maybe `MaxLines` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
