[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Johan Vikström via Phabricator via cfe-commits]

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