[clangd] Add a FileSymbols container that manages symbols from multiple files.

[sammccall]

clangd/index/FileSymbols.h

39 ↗

(On Diff #126936)

Consider just accepting update(Path, nullptr) for this (or make callers pass an empty slab).
This can make callsites that might call one or the other conditionally, less awkward.

unittests/clangd/IndexTests.cpp

124 ↗

(On Diff #126936)

Can you move these tests into a separate cpp file?
The code overlap is really minimal, and they test things from different headers.

[ioeric]

• Address review comments

[ioeric]

• fix HEADER_GUARD

clang-tools-extra/trunk/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   JSONExpr.cpp
   JSONRPCDispatcher.cpp
   Logger.cpp
   Protocol.cpp
   ProtocolHandlers.cpp
   Trace.cpp
-  index/MemIndex.cpp
+  index/FileSymbols.cpp
   index/Index.cpp
+  index/MemIndex.cpp
   index/SymbolCollector.cpp
   index/SymbolYAML.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangFormat
   clangFrontend

clang-tools-extra/trunk/clangd/index/FileSymbols.h

+//===--- FileSymbols.h - Symbols from files. ---------------------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_FILESYMBOLS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_FILESYMBOLS_H
+
+#include "../Path.h"
+#include "Index.h"
+#include "llvm/ADT/StringMap.h"
+#include <mutex>
+
+namespace clang {
+namespace clangd {
+
+/// \brief A container of Symbols from several source files. It can be updated
+/// at source-file granularity, replacing all symbols from one file with a new
+/// set.
+///
+/// This implements a snapshot semantics for symbols in a file. Each update to a
+/// file will create a new snapshot for all symbols in the file. Snapshots are
+/// managed with shared pointers that are shared between this class and the
+/// users. For each file, this class only stores a pointer pointing to the
+/// newest snapshot, and an outdated snapshot is deleted by the last owner of
+/// the snapshot, either this class or the symbol index.
+///
+/// The snapshot semantics keeps critical sections minimal since we only need
+/// locking when we swap or obtain refereces to snapshots.
+class FileSymbols {
+public:
+  /// \brief Updates all symbols in a file. If \p Slab is nullptr, symbols for
+  /// \p Path will be removed.
+  void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab);
+
+  // The shared_ptr keeps the symbols alive
+  std::shared_ptr<std::vector<const Symbol *>> allSymbols();
+
+private:
+  mutable std::mutex Mutex;
+
+  /// \brief Stores the latest snapshots for all active files.
+  llvm::StringMap<std::shared_ptr<SymbolSlab>> FileToSlabs;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_FILESYMBOLS_H

clang-tools-extra/trunk/clangd/index/FileSymbols.cpp

+//===--- FileSymbols.cpp - Symbols from files. ------------------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FileSymbols.h"
+#include "clang/Index/IndexingAction.h"
+
+namespace clang {
+namespace clangd {
+
+void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Slab) {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  if (!Slab)
+    FileToSlabs.erase(Path);
+  else
+    FileToSlabs[Path] = std::shared_ptr<SymbolSlab>(Slab.release());
+}
+
+std::shared_ptr<std::vector<const Symbol *>> FileSymbols::allSymbols() {
+  // The snapshot manages life time of symbol slabs and provides pointers of all
+  // symbols in all slabs.
+  struct Snapshot {
+    std::vector<const Symbol *> Pointers;
+    std::vector<std::shared_ptr<SymbolSlab>> KeepAlive;
+  };
+  auto Snap = std::make_shared<Snapshot>();
+  {
+    std::lock_guard<std::mutex> Lock(Mutex);
+
+    for (const auto &FileAndSlab : FileToSlabs) {
+      Snap->KeepAlive.push_back(FileAndSlab.second);
+      for (const auto &Iter : *FileAndSlab.second)
+        Snap->Pointers.push_back(&Iter.second);
+    }
+  }
+  auto *Pointers = &Snap->Pointers;
+  // Use aliasing constructor to keep the snapshot alive along with the
+  // pointers.
+  return {std::move(Snap), Pointers};
+}
+
+} // namespace clangd
+} // namespace clang

clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 get_filename_component(CLANGD_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clangd REALPATH)
 include_directories(
   ${CLANGD_SOURCE_DIR}
   )
 
 add_extra_unittest(ClangdTests
   ClangdTests.cpp
   CodeCompleteTests.cpp
   ContextTests.cpp
+  FileSymbolsTests.cpp
   FuzzyMatchTests.cpp
   IndexTests.cpp
   JSONExprTests.cpp
   TestFS.cpp
   TraceTests.cpp
   SymbolCollectorTests.cpp
   )
 

clang-tools-extra/trunk/unittests/clangd/FileSymbolsTests.cpp

+//===-- FileSymbolsTests.cpp  -------------------------*- C++ -*-----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "index/FileSymbols.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::UnorderedElementsAre;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+
+Symbol symbol(llvm::StringRef ID) {
+  Symbol Sym;
+  Sym.ID = SymbolID(ID);
+  Sym.QualifiedName = ID;
+  return Sym;
+}
+
+void addNumSymbolsToSlab(int Begin, int End, SymbolSlab *Slab) {
+  for (int i = Begin; i <= End; i++)
+    Slab->insert(symbol(std::to_string(i)));
+}
+
+std::vector<std::string>
+getSymbolNames(const std::vector<const Symbol *> &Symbols) {
+  std::vector<std::string> Names;
+  for (const Symbol *Sym : Symbols)
+    Names.push_back(Sym->QualifiedName);
+  return Names;
+}
+
+TEST(FileSymbolsTest, UpdateAndGet) {
+  FileSymbols FS;
+  EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
+
+  auto Slab = llvm::make_unique<SymbolSlab>();
+  addNumSymbolsToSlab(1, 3, Slab.get());
+
+  FS.update("f1", std::move(Slab));
+
+  EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
+              UnorderedElementsAre("1", "2", "3"));
+}
+
+TEST(FileSymbolsTest, Overlap) {
+  FileSymbols FS;
+
+  auto Slab = llvm::make_unique<SymbolSlab>();
+  addNumSymbolsToSlab(1, 3, Slab.get());
+
+  FS.update("f1", std::move(Slab));
+
+  Slab = llvm::make_unique<SymbolSlab>();
+  addNumSymbolsToSlab(3, 5, Slab.get());
+
+  FS.update("f2", std::move(Slab));
+
+  EXPECT_THAT(getSymbolNames(*FS.allSymbols()),
+              UnorderedElementsAre("1", "2", "3", "3", "4", "5"));
+}
+
+TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
+  FileSymbols FS;
+
+  auto Slab = llvm::make_unique<SymbolSlab>();
+  addNumSymbolsToSlab(1, 3, Slab.get());
+
+  FS.update("f1", std::move(Slab));
+
+  auto Symbols = FS.allSymbols();
+  EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
+
+  FS.update("f1", nullptr);
+  EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre());
+
+  EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
+