[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 7 inline comments as done.
sammccall added a comment.

In https://reviews.llvm.org/D51605#1222636, @hokein wrote:

> Thanks for cleaning it up! I admit that `SymbolOccurrences` is a loong name. A few nits.


Yeah, I hope you don't mind! The references stuff is awesome, I'm hoping to make it work with static index soon!



================
Comment at: clangd/index/FileIndex.cpp:120
+      auto &SymRefs = Sym.second;
+      std::sort(SymRefs.begin(), SymRefs.end());
+      std::copy(SymRefs.begin(), SymRefs.end(), back_inserter(RefsStorage));
----------------
hokein wrote:
> So the sort is intended to make returned refs ordered?
Yeah, I'm worried about the fact that we have no ranking at all, so the natural implementation of refs() with limit will return randomly different results after each dynamic index rebuild.
Added a comment here, also happy to remove the sorting if you prefer.


================
Comment at: clangd/index/Index.h:367
+    return sizeof(*this) + Arena.getTotalMemory() +
+           sizeof(Refs.front()) * Refs.size();
   }
----------------
hokein wrote:
> we should be careful if `Refs` is empty. Calling `front()` on empty container is undefined behavior. 
the arg to sizeof() is an unevaluated context, so it never actually gets called (just the type is evaluated).
Changed to sizeof(value_type) for clarity though.


================
Comment at: clangd/index/Merge.cpp:13
 #include "../Logger.h"
+#include "Merge.h"
 #include "llvm/ADT/STLExtras.h"
----------------
ioeric wrote:
> nit: shouldn't the main header be sorted higher? 
> 
> We should also put all #includes in one block so that clang-format can sort them properly.
Ah, <set> was confusing clang-format's main-header heuristic.


================
Comment at: unittests/clangd/FileIndexTests.cpp:341
+  std::vector<Ref> Results;
+  Index.refs(Request, [&Results](const Ref &O) { Results.push_back(O); });
 
----------------
hokein wrote:
> My fault here. This is not safe, we need to store the underlying `FileURI` data.
well, the contract on SymbolIndex doesn't say so, but in this concrete case, we know the implementation so it's not too bad.

In any case, avoided the issue (changed getRefPaths -> getRefs which returns a slab)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51605