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 

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?

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to