ioeric added inline comments.
================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:67
+ // XXX this is just to make running the tool fast during dev!
+ bool BeginInvocation(CompilerInstance &CI) override {
+ const auto &Inputs = CI.getInvocation().getFrontendOpts().Inputs;
----------------
sammccall wrote:
> hokein wrote:
> > It is fine for debugging, but I think we don't want this behavior by
> > default.
> >
> > global-symbol-builder also supports running a single TU
> > (`global-symbol-builder /path/to/file`), which is sufficient for debugging,
> > I think?
> >
> Yeah, I'm not going to check this in, thus the XXX comment :-)
>
> Single TU isn't enough - it doesn't test the reducer. On the other hand the
> full compilation database is too big. So this option would actually be
> useful! But it doesn't belong here.
Drive-by: you could also run the tool in the default standalone mode and
provide a list of files.
================
Comment at: clangd/index/Merge.cpp:93
S.Detail = Scratch;
} else if (L.Detail)
+ /* Current state: S.Detail = O.Detail */;
----------------
`else if (S.Detail)`?
`/*Current state: S.Detail = S.Detail*/`?
================
Comment at: clangd/index/SymbolCollector.cpp:210
+ BasicSymbol = addDeclaration(*ND, std::move(ID));
+ if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
+ addDefinition(*cast<NamedDecl>(ASTNode.OrigD), *BasicSymbol);
----------------
It seems that we store definition even if it's the same as declaration, which
might be true for most classes? This is probably fine, but it would be worth
documenting. Or maybe consider not storing the same location twice?
================
Comment at: clangd/index/SymbolCollector.cpp:236
+ std::string FileURI;
+ // FIXME: we may want a different "canonical" heuristic than clang chooses.
+ if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, FileURI))
----------------
Could you elaborate a bit more on this one? What is the current heuristic, and
what would we prefer?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:49
MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
----------------
Fallout of a git merge?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:108
+ "-std=c++11",
+ "-include",
+ llvm::sys::path::filename(TestHeaderName),
----------------
neat!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42942
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
