[PATCH] D42942: [clangd] Collect definitions when indexing.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.

Lg



================
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;
----------------
hokein wrote:
> 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 :(.
You are right :) But I don't think this matters much if you only want to test reduction.


================
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);
----------------
sammccall wrote:
> hokein wrote:
> > 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.
> Documented that these may be the same.
> We wouldn't actually save any memory by avoiding saving this twice - the filename is a stringref to the same data, and the offsets get stored even if they're null.
I'm less concerned about space. I think this would also make it easy to check whether a symbol has a separate definition (e.g. we might want to do this for go-to-definition/go-to-declaration). A comparison would also work but seems less natural. But I'm not sure, up tp you :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42942