hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:119
void addToken(SourceLocation Loc, const NamedDecl *D) {
+ if (D->isInvalidDecl())
+ return;
----------------
This means if we won't emit most of symbols if the AST is ill-formed? I'm not
sure whether we should include it in this patch, it seems out of scope. could
we leave it out in this patch?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:68
+
+/// Removes every line where \c Highlightings is the same as \c
+/// PrevHighlightings. If \c PrevHighlightings has lines that does not exist
----------------
The comment seems a bit out of date (as the code is updated during review), and
it should mention the diff strategy of this function.
```
// Return a line-by-line diff between two highlightings.
// - if the tokens on a line are the same in both hightlightings, we omit this
line
// - if a line exists in NewHighligtings but not in OldHighligtings, we emit
the tokens on this line
// - if a line not exists in NewHighligtings but in OldHighligtings, we emit
an empty line (to tell client not highlighting this line)
```
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:53
}
+ return ExpectedTokens;
+}
----------------
nit: let's sort the tokens here to make the result deterministic.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:59
+ std::vector<int> EmptyLines(EmptyRanges.size());
+ for (unsigned I = 0, End = EmptyRanges.size(); I < End; ++I)
+ EmptyLines[I] = EmptyRanges[I].start.line;
----------------
nit: you could use for-range loop to simplify the code.
```
for (const auto& EmptyRange : EmptyRanges)
EmptyLines.push_back(EmptyRange.start.line);
```
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:75
+ std::vector<HighlightingToken> OldActualTokens = getActualTokens(OldTest);
+ std::vector<HighlightingToken> NewActualTokens = getActualTokens(NewTest);
+ std::vector<HighlightingToken> ExpectedTokens = getExpectedTokens(NewTest);
----------------
nit: since `OldActualTokens` and `NewActualTokens` are used only once in
`diffHighlightings`, I'd inline them there (instead of creating two new
variables).
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:88
+ for (auto &LineTokens : ExpectedLines) {
+ llvm::sort(LineTokens.second);
+ ExpectedLinePairHighlighting.push_back(
----------------
if we do sort in `getExpectedTokens`, we don't need this here.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:274
+ // The first entry is the old code. The second entry is the new code.
+ std::vector<std::pair<const char *, const char *>> TestCases{{
+ R"cpp(
----------------
nit: Use an explicit struct, then you wouldn't need a comment.
```
struct {
llvm::StringRef OldCode;
llvm::StringRef NewCode;
} TestCases[] = {
...
}
```
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:275
+ std::vector<std::pair<const char *, const char *>> TestCases{{
+ R"cpp(
+ int A
----------------
could we add one more case?
```
// Before
int a;
int b;
int c;
// After
int a;
int $Variable[[new_b]];
int c;
```
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:327
+
+ for (auto Test : TestCases) {
+ checkDiffedHighlights(Test.first, Test.second);
----------------
nit: `const auto&`
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
