sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:172
+ const HighlightingToken &Rhs) {
+ return Lhs.R.start.line == Rhs.R.start.line
+ ? Lhs.R.start.character < Rhs.R.start.character
----------------
I think this is just Lhs.R.start < Lhs.R.end
(if it's not, we should add the `operator<`)
is it enforced somewhere that you don't have two highlights at the same spot?
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:190
+ // highlightings for old ASTs)
+ std::lock_guard<std::mutex> Lock(PrevMutex);
+ // The files are different therefore all highlights differ.
----------------
holding the lock while doing all the diffing seems dubious
You could reasonably put the cache as a map<file, highlights> in ClangdServer,
then you don't have to deal with not having the cache for the current file.
You'd need to lock the map itself, but note that no races on updating
individual entries are possible because onMainAST is called synchronously from
the (per-file) worker thread.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+ // PrevHighlightings and Highlightings.
+ int I = 0, PI = 0, EndI = Highlightings.size(),
+ EndP = PrevHighlightings.size();
----------------
Whatever you do about storage, please pull out the diff(old, new) function so
you can unit test it.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:207
+ // PrevHighlightings and Highlightings.
+ int I = 0, PI = 0, EndI = Highlightings.size(),
+ EndP = PrevHighlightings.size();
----------------
sammccall wrote:
> Whatever you do about storage, please pull out the diff(old, new) function so
> you can unit test it.
(llvm uses `unsigned` for indices. It's a terrible idea, but it's used fairly
consistently...)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:209
+ EndP = PrevHighlightings.size();
+ while (I < EndI && PI < EndP) {
+ const HighlightingToken &Current = Highlightings[I];
----------------
I can believe this is correct, but it's hard to verify as it's a bit monolithic
and... index-arithmetic-y.
could you factor out something like:
```
using TokRange = ArrayRef<HighlightingToken>;
// The highlights for the current line.
TokRange OldLine = {PrevHighlightings.data(), 0}, NewLine =
{Highlightings.data(), 0};
// iterate over lines until we run out of data
for (unsigned Line = 0; OldLine.end() < PrevHighlightings.end() ||
NewLine.end() < PrevHighlightings.end(); ++Line) {
// Get the current line highlights
OldLine = getLineRange(PrevHighlightings, Line, OldLine);
NewLine = getLineRange(Highlightings, Line, NewLine);
if (OldLine != NewLine)
emitLine(NewLine, Line);
}
// get the highlights for Line, which follows PrevLine
TokRange getLineRange(TokRange AllTokens, unsigned Line, TokRange PrevLine) {
return makeArrayRef(PrevLine.end(), AllTokens.end()).take_while( Tok =>
Tok.R.start.line == Line);
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
