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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
