[PATCH] D51279: [clangd] Implement findOccurrences interface in dynamic index.

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 31 03:48:53 PDT 2018

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

This basically looks good to go (some fixes needed but they're pretty clear I think let me know if not!)

Comment at: clangd/index/FileIndex.cpp:63
+  auto Occurrences = Collector.takeOccurrences();
+  vlog("index for AST: \n"
+       "  symbol slab: {0} symbols, {1} bytes\n"
for this log to be useful, you might want to include the filename.
You can get it from the sourcemanager on the ASTContext.

Comment at: clangd/index/MemIndex.cpp:35
 std::unique_ptr<SymbolIndex> MemIndex::build(SymbolSlab Slab) {
   auto Idx = llvm::make_unique<MemIndex>();
This is still implicitly creating an index with no occurrences. Did you mean to accept a SymbolOccurrencesSlab here?

Comment at: clangd/index/MemIndex.h:23
-  /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
-  /// accessible as long as `Symbols` is kept alive.
-  void build(std::shared_ptr<std::vector<const Symbol *>> Symbols);
+  using OccurrenceMap =
+      llvm::DenseMap<SymbolID, std::vector<const SymbolOccurrence *>>;
can you add an explanation to this?

Comment at: clangd/index/Merge.cpp:94
+    // instead.
+    llvm::DenseSet<llvm::StringRef> DynamicIndexFileURIs;
+    Dynamic->findOccurrences(Req, [&](const SymbolOccurrence &O) {
Unfortunately this is not safe, the backing strings may not live long enough.
This should be a StringSet (by value) instead.

Comment at: clangd/index/Merge.cpp:100
+    Static->findOccurrences(Req, [&](const SymbolOccurrence &O) {
+      if (DynamicIndexFileURIs.find(O.Location.FileURI) !=
+          DynamicIndexFileURIs.end())
or just DynamicIndexFileURIs.count(O.Location.FileURI) and rely on the implicit conversion to bool

Comment at: clangd/index/SymbolCollector.cpp:227
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+  SymbolOccurrenceKind Kind;

isn't this just 

`return Roles & AllOccurrenceKinds` with some casts?
(I'd put AllOccurrenceKinds into the header, but it could also be a constant here)

Comment at: clangd/index/SymbolCollector.h:120
+  using DeclOccurrence = std::pair<SourceLocation, index::SymbolRoleSet>;
+  llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences;
move these up next to ReferencedDecls and ReferencedMacros so the comment applies to them too?

Similarly, move SymbolOccurrenceSlab next to the SymbolSlab::Builder? These have strong parallels.

Comment at: clangd/index/SymbolCollector.h:122
+  llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>> DeclOccurrences;
+  // All symbol occurrences collected from the AST, assembled on finish().
+  // Only symbols declared in preamble (from #inclues) and references from the
I'm not sure this works - IIRC can use a SymbolCollector for multiple TUs, with finish() called at the end of each one.
I think you need to (incrementally) build in finish(), and freeze in takeOccurrences().

Comment at: unittests/clangd/TestTU.h:41
+  static TestTU withAllCode(llvm::StringRef HeaderCode, llvm::StringRef Code,
+                            llvm::StringRef Filename = "") {
We've avoided adding this in the past because it's less readable. Please assign the fields separately instead.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list