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