sammccall marked an inline comment as done.
sammccall added inline comments.


================
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:
> 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 :)
Yeah sometimes we'll definitely want to check if they're different, but other 
times we'll want to use one without caring if they are. Comparison seems less 
surprising than either of the fields sometimes being empty even though data is 
available - that seems likely to lead to bugs I think.


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