[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for the split & refactoring! This looks nice. Just nits, plus some thoughts on the test.



================
Comment at: clangd/index/Index.cpp:11
 #include "Index.h"
-
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/SHA1.h"
----------------
changes in this file can now be reverted I think


================
Comment at: clangd/index/Index.h:128
+  /// \brief Matches symbols in the index fuzzily and applies \p Callback on
+  /// each matched symbol.
+  ///
----------------
nit: "... before returning".

Just to be explicit that this is a synchronous API.


================
Comment at: clangd/index/Index.h:130
+  ///
+  /// Returns true if all candidates are matched.
+  virtual bool
----------------
This is a little vague - Returns true if the result list is complete, false if it was truncated due to MaxCandidateCount?

Might be clearer to invert it - Returns true if the result list was truncated - up to you.


================
Comment at: clangd/index/Index.h:132
+  virtual bool
+  fuzzyFind(const FuzzyFindRequest &Req,
+            std::function<void(const Symbol &)> Callback) const = 0;
----------------
context patch has landed, so this should now take const Context& as first parameter


================
Comment at: clangd/index/Index.h:136
+  // FIXME: add interfaces for more index use cases:
+  //  - Symbol getSymbolInfo(llvm::StringRef USR);
+  //  - getAllOccurrences(llvm::StringRef USR);
----------------
USRs will rather be SymbolIDs


================
Comment at: clangd/index/MemIndex.cpp:38
+      // Find all symbols that contain the query, igoring cases.
+      // FIXME: use better matching algorithm, e.g. fuzzy matcher.
+      if (StringRef(StringRef(Sym->QualifiedName).lower())
----------------
you can easily do this now - it shouldn't even be more lines of code.
Up to you of course.


================
Comment at: unittests/clangd/IndexTests.cpp:29
+
+struct CountedSymbolSlab {
+  CountedSymbolSlab() = delete;
----------------
nit: given the name, I actually think inheriting from Slab might be clearer.

Though I'm not sure we actually need this, see `weak_ptr` part of next comment.


================
Comment at: unittests/clangd/IndexTests.cpp:41
+
+class MemIndexTest : public ::testing::Test {
+protected:
----------------
This is a nice test, the one weakness I see is it's hard to read the cases in isolation as there's quite a lot of action-at-a-distance.
I think we can eliminate the base class without adding much boilerplate, which would make the tests more self-contained:
  - `Index` can be owned by the test
  - `match()` is good, but moving `Index.build()` call into the test, and accepting `Index` as a param means this interaction is more obvious and `match` can be a free function.
  - `CountedSymbolSlab` can more directly be replaced by a `weak_ptr`, which can tell you whether a `shared_ptr` is alive
  - `generateNumSymbols` (already) doesn't need to be a member. Having it optionally emit a weak_ptr to its return value would simplify the tests a little.

So MemIndexSymbolsRecycled would look something like

    Index I;
    std::weak_ptr<vector<const Symbol*>> Symbols;
    I.build(generateNumSymbols(0, 10), &Symbols);
    EXPECT_THAT(match(I, Req), ElementsAre("7"));
    EXPECT_FALSE(Symbols.expired());
    I.build(generateNumSymbols(0,0));
    EXPECT_TRUE(Symbols.expired());

WDYT?


================
Comment at: unittests/clangd/IndexTests.cpp:62
+      Slab->Slab.insert(symbol(std::to_string(i)));
+    auto *Symbols = new std::vector<const Symbol *>();
+
----------------
This is a leak - the returned shared_ptr doesn't own this vector.

(I'm actually fine with this for simplicity if it's documented - but we might be running leak checkers that complain about it?)

In principle you need to make_shared a struct containing both the vector and the slab.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548