jvikstrom marked an inline comment as done.
jvikstrom 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
----------------
sammccall wrote:
> 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?
It's not enforced anywhere, it depends on how the HighlightingTokenCollector
is implemented, it should not currently take any multiples of tokens at the
same spot.
Even if there are tokens at the same location it should still satisfy Compare
though?
================
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.
----------------
sammccall wrote:
> 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.
I did not know onMainAST was called synchronously per file.
I feel like after a while a lot of memory is going to be consumed by keeping
ASTs for files that were closed long ago in the cache (say when you've opened
1000 files in a session, all those files will have one Highlightings entry here)
Could solve that with LRU (just keep a set of pair<long long, std::string>
(unix time of adding/editing to cache and filename)... would actually have to
be a struct, would need to make the equal operator check the filename only)
But maybe that isn't a big concern/a concern at all?
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
