hokein 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;
----------------
ioeric wrote:
> 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.
However the default standalone mode is not multiple thread :(.
================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:159
+ // Output phase: emit YAML for result symbols.
for (const auto &Sym : UniqueSymbols)
+ llvm::outs() << SymbolToYAML(Sym);
----------------
nit: we could get rid of the loop by using `SymbolsToYAML(UniqueSymbols)` .
================
Comment at: clangd/index/SymbolCollector.cpp:137
+// FIXME: EndOffset is inclusive (closed range), and should be exclusive.
+// FIXME: Because the underlying ranges are token ranges, this code chops the
+// last token in half if it contains multiple characters.
----------------
That's bad, thanks for finding it.
================
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);
----------------
ioeric wrote:
> 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?
I think it is fine to store the same location twice. We can't tell whether the
CanonicalLoc is a definition or not.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42942
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
