[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 17 01:53:33 PDT 2019
hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+ // ArrayRefs to the current line in the highlights.
+ ArrayRef<HighlightingToken> NewLine(Highlightings.data(),
+ Highlightings.data()),
----------------
jvikstrom wrote:
> hokein wrote:
> > IIUC, we are initializing an empty ArrayRef, if so, how about using `NewLine(Highlightings.data(), /*length*/0)`? `NewLine(Highlightings.data(), Highlightings.data())` is a bit weird, it took me a while to understand the purpose.
> I couldn't do that because the `ArrayRef(const T *data, size_t length)` and `ArrayRef(const T *begin, const T *end)` were ambiguous. Added a cast to cast 0 to a size_t which solved it.
you could also do it like `NewLine(Highlightings.data(), /*length*/0UL);`, which saves you a `static_cast<>`.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:281
+ )cpp"}},
+ {{5},
+ {
----------------
so the empty lines are stored separately, which is not easy to figure out from the testing code snippet. I think we should make the empty lines more obvious in the code snippet (maybe use an `Empty` annotations?) , and explicitly verify the empty lines.
What do you think refining the test like below, just annotate the diff tokens? I find it is easy to spot the diff tokens.
```
struct Testcase {
Code Before;
Code After;
};
std::vector<TestCase> cases = {
{
R"cpp(
int a;
int b;
int c;
)cpp",
R"cpp(
int a;
$Empty[[]] // int b
int $Variable[[C]];
)cpp"
}
}
oldHighlightings = getSemanticHighlightings(OldAST);
newHighlightings = getSemanticHighlightings(NewAST);
diffHighlightings = diff...;
// verify the diffHighlightings has the expected empty lines ("Empty" annotations).
// verify the diffHighlightings has the expected highlightings (the regular annotations);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
More information about the cfe-commits
mailing list