sammccall added a comment.
This looks pretty good!
================
Comment at: clangd/index/Index.h:376
+
+ llvm::ArrayRef<SymbolOccurrence> find(const SymbolID &ID) const {
+ auto It = Occurrences.find(ID);
----------------
assert frozen?
looking up in a non-frozen array is probably a mistake.
if we choose to optimize this, it probably won't be possible.
================
Comment at: clangd/index/Index.h:377
+ llvm::ArrayRef<SymbolOccurrence> find(const SymbolID &ID) const {
+ auto It = Occurrences.find(ID);
+ if (It == Occurrences.end())
----------------
return Occurrences.lookup(ID)?
================
Comment at: clangd/index/SymbolCollector.cpp:227
+SymbolOccurrenceKind ToOccurrenceKind(index::SymbolRoleSet Roles) {
+ SymbolOccurrenceKind Kind;
----------------
nit: toOccurrenceKind
================
Comment at: clangd/index/SymbolCollector.cpp:229
+ SymbolOccurrenceKind Kind;
+ for (auto Mask :
+ {SymbolOccurrenceKind::Declaration, SymbolOccurrenceKind::Definition,
----------------
If you want to filter out the unsupported bits, maybe just add an explicit
`AllOccurrenceKinds` constant to the header file, and `return
AllOccurrenceKinds & Roles` here? (plus casts)
================
Comment at: clangd/index/SymbolCollector.cpp:328
+ if ((static_cast<unsigned>(Opts.OccurrenceFilter) & Roles) &&
+ SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+ DeclOccurrences[ND].emplace_back(Loc, Roles);
----------------
just compute the spelling loc once and reuse?
================
Comment at: clangd/index/SymbolCollector.cpp:329
+ SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
+ DeclOccurrences[ND].emplace_back(Loc, Roles);
+
----------------
you get the spelling loc on the previous line to check for mainfile - so surely
we should be using spelling loc here?
================
Comment at: clangd/index/SymbolCollector.cpp:455
+
+ for (auto& It : DeclOccurrences) {
+ if (auto ID = getSymbolID(It.first)) {
----------------
nit: const auto& for clarity since we're not mutating
================
Comment at: clangd/index/SymbolCollector.cpp:460
+ std::string FileURI;
+ if (auto DeclLoc = getTokenLocation(LocAndRole.first,
+ ASTCtx->getSourceManager(), Opts,
----------------
so this seems maybe gratuitously inefficient, we're copying the filename then
going through the URI conversion dance for each reference - even though the
filename is the same for each.
consider splitting out part of `getTokenLocation` into
`getTokenRange(SymbolLocation&)` and only calling that here.
================
Comment at: clangd/index/SymbolCollector.h:54
// Populate the Symbol.References field.
bool CountReferences = false;
// Every symbol collected will be stamped with this origin.
----------------
this should be next to OccurrenceFilter, they're very closely related (the name
mismatch is a little unfortunate)
================
Comment at: clangd/index/SymbolCollector.h:121
+ using DeclOccurrence = std::pair<SourceLocation, index::SymbolRoleSet>;
+ llvm::DenseMap<const NamedDecl*, std::vector<DeclOccurrence>>
DeclOccurrences;
+ // All symbol occurrences collected from the AST, assembled on finish().
----------------
please move next to ReferencedDecls/ReferencedMacros so the comment applies to
this too
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:466
+ SymbolOccurrences.find(findSymbol(Symbols, "Foo").ID),
+ testing::UnorderedPointwise(OccurrenceRange(), Main.ranges("foo")));
+ EXPECT_THAT(
----------------
this is cute - if possible, consider adding a matcher factory function for
readability here, so you can write `EXPECT_THAT(...,
HaveRanges(Main.ranges("foo"))`
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
