[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/Index.h:310
+
+  llvm::ArrayRef<SymbolOccurrence> findOccurrences(const SymbolID &ID) const {
+    auto It = SymbolOccurrences.find(ID);
----------------
As discussed offline: the merge of occurrences into SymbolSlab seems problematic to me.
On the consumer side, we have a separation between Symbol APIs and SymbolOccurrence APIs - they don't really interact.
The Symbol type can often only be used with SymbolSlab, and so including occurrences drags them into the mess for consumers that don't care about them.

For producers (index implementations), they will usually have both and they may want to share arena storage. But this probably doesn't matter much, and if it does we can use another mechanism (like allowing SymbolSlabBuilder and SymbolOccurrenceSlab to share UniqueStringSaver)


================
Comment at: clangd/index/SymbolCollector.cpp:439
       SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
----------------
note that here we've done basically all the work needed to record the occurrence.

If you add a DenseMap<Decl*, {SourceLocation, SymbolRole}> then you'll have enough info at the end to fill in the occurrences, like we do with referenceddecls -> references.


================
Comment at: clangd/index/SymbolCollector.h:59
+      // If none, all symbol occurrences will be collected.
+      llvm::Optional<llvm::DenseSet<SymbolID>> IDs = llvm::None;
+    };
----------------
ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Could you elaborate on what this option will be used for? How do we know in advance which symbols we're interested in?
> > This is used for finding references in the AST as a part of the xref implementation, basically the workflow would be:
> > 
> > 1. find SymbolIDs of the symbol under the cursor, using `DeclarationAndMacrosFinder`
> > 2. run symbol collector to find all occurrences in the main AST with all SymbolIDs in #1
> > 3. query the index, to get more occurrences
> > 4. merge them  
> Can we instead find all the occurences in  `DeclarationAndMacrosFinder` directly?
> Extra run of `SymbolCollector` means another AST traversal, which is slow by itself, and SymbolCollector s designed for a much more hairy problem, its interface is just not nicely suited for things like only occurrences. The latter seems to be a simpler problem, and we can have a simpler interface to solve it (possibly shared between SymbolCollector and DeclarationAndMacrosFinder). WDYT?
> 
> 
Yeah, I don't think we need this.
For "find references in the AST" we have an implementation in XRefs for highlights which we don't need to share.


================
Comment at: clangd/index/SymbolCollector.h:40
   struct Options {
+    struct CollectSymbolOptions {
+      bool CollectIncludePath = false;
----------------
Not sure this split is justified.
if IDs goes away (see below), all that's left can be represented in a SymbolOccurenceKind filter (which is 0 to collect no occurrences)


================
Comment at: clangd/index/SymbolCollector.h:76
+    // If not null, SymbolCollector will collect symbol occurrences.
+    const CollectOccurrenceOptions *OccurrenceOpts;
   };
----------------
collecting symbols doesn't actually need to be optional I think - it's the core responsibility of this class, and "find occurrences of a decl in an ast" can be implemented more easily in other ways


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385