[PATCH] D49658: [clangd] Index Interfaces for Xrefs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly LG.

I think we can simplify in a couple of places, and you should decide if this patch is *implementing* the new index operation or not. (Currently it implements it for 1/3 implementations, which doesn't seem that useful).



================
Comment at: clangd/index/FileIndex.cpp:111
+    llvm::function_ref<void(const SymbolOccurrence &)> Callback) const {
+  // FIXME(hokein): implement it.
+}
----------------
add a log?


================
Comment at: clangd/index/FileIndex.cpp:111
+    llvm::function_ref<void(const SymbolOccurrence &)> Callback) const {
+  // FIXME(hokein): implement it.
+}
----------------
sammccall wrote:
> add a log?
Can we either implement this for FileIndex/MemIndex/MergedIndex, or none of them?
ATM the scope of the patch is unclear. If we don't implement it, then no need to add Slab etc. If we do, then we get 3 callsites to make sure all that stuff actually works well :-)


================
Comment at: clangd/index/Index.cpp:138
+raw_ostream &operator<<(raw_ostream &OS, SymbolOccurrenceKind K) {
+  if (K == SymbolOccurrenceKind::Unknown)
+    return OS << "unknown";
----------------
this is a bitfield, there are at least 8 legal values.

If you intend each occurrence to have only one type, and only the filter to have multiple values, these should probably be different types.

Do we actually need operator<< in this patch, or is the default (print the number) OK for tests?


================
Comment at: clangd/index/Index.h:313
+  // The location of the occurrence.
+  // WARNING: Location does not own the underlying data - FileURI are owned by a
+  // SymbolOccurrenceSlab. Copies are shallow.
----------------
Hmm, remove the bit about slab? That's only going to be true in a smaller set of cases than with Symbol.
Not owning the data is a good note, but probably belongs on the struct rather than the member.


================
Comment at: clangd/index/Index.h:345
+
+  // A mutable container that can 'freeze' to SymbolOccurrenceSlab.
+  // The frozen OccurrenceSlab will use less memory.
----------------
Again, I'm not sure the builder/slab split is justifiable here. Can we start without it? Cost is ~8 bytes per unique filename, which should be pretty trivial.


================
Comment at: clangd/index/Index.h:429
+  ///
+  /// The API is not responsible to rank results.
+  /// The returned result must be deep-copied if it's used outside Callback.
----------------
nit: `Results are returned in arbitrary order.`?
(Usually we want to document from the POV of a caller, rather than an implementation)


================
Comment at: clangd/index/MemIndex.cpp:97
+    for (const auto& Occurrence : Occurrences.find(ID)) {
+      if (static_cast<uint32_t>(Req.Filter & Occurrence.Kind)) {
+       Callback(Occurrence);
----------------
cast to bool rather than int32_t?


================
Comment at: clangd/index/MemIndex.h:26
+  void build(std::shared_ptr<std::vector<const Symbol *>> Symbols,
+             SymbolOccurrenceSlab OccurrenceSlab = {});
 
----------------
nit: I'm not sure default args make sense here unless we intend for this to be forever an optional feature


================
Comment at: clangd/index/MemIndex.h:29
   /// \brief Build index from a symbol slab.
   static std::unique_ptr<SymbolIndex> build(SymbolSlab Slab);
 
----------------
occurrences here too


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49658