ioeric added inline comments.
================
Comment at: clangd/index/SymbolCollector.cpp:386
const Symbol *BasicSymbol = Symbols.find(*ID);
- if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
- BasicSymbol = addDeclaration(*ND, std::move(*ID));
- else if (isPreferredDeclaration(OriginalDecl, Roles))
- // If OriginalDecl is preferred, replace the existing canonical
- // declaration (e.g. a class forward declaration). There should be at most
- // one duplicate as we expect to see only one preferred declaration per
- // TU, because in practice they are definitions.
- BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
-
- if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
- addDefinition(OriginalDecl, *BasicSymbol);
+ auto shouldIndexSymbol = [&](SourceLocation TargetLoc) {
+ return shouldIndexFile(SM, SM.getFileID(TargetLoc), Opts,
----------------
`shouldIndexLoc` seems more accurate?
================
Comment at: clangd/index/SymbolCollector.cpp:392
+ if (!BasicSymbol) {
+ auto DeclSLoc = findNameLoc(ND);
+ if (shouldIndexSymbol(DeclSLoc))
----------------
`DeclSLoc` is a bit confusing (SLoc is used somewhere else). Maybe `DeclSymLoc`
or simply `DeclLoc`?
================
Comment at: clangd/index/SymbolCollector.cpp:397
+ }
+ if (isPreferredDeclaration(OriginalDecl, Roles)) {
+ auto DeclSLoc = findNameLoc(&OriginalDecl);
----------------
We would be doing redundant work if `OriginalDecl == ND`?
================
Comment at: clangd/index/SymbolCollector.cpp:409
+ auto DefLoc = findNameLoc(&OriginalDecl);
+ if (!shouldIndexSymbol(DefLoc) && !BasicSymbol)
+ return true;
----------------
Could you document here why we are also checking `!BasicSymbol ` here?
================
Comment at: clangd/index/SymbolCollector.cpp:571
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
+ SourceLocation DeclSLoc,
SymbolID ID) {
----------------
nit: `s/DeclSLoc/DeclLoc`? and maybe add documentation about why we need this?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:598
+ Pair(findSymbol(Symbols, "Foo2").ID,
+ RefsInFiles(Files({IndexedHeaderURI}))),
+ Pair(findSymbol(Symbols, "Foo3").ID,
----------------
I think making `RefsInFile` take a single file and using it in combination with
`anyOf`/`allOf` would be more straightforward.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54300/new/
https://reviews.llvm.org/D54300
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
