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

Reply via email to