sammccall added a comment.
Thanks for the comments!
And sorry for the delay, I was haunted by useless gtest messages, which
https://reviews.llvm.org/D43091 should fix.
================
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);
----------------
hokein wrote:
> nit: we could get rid of the loop by using `SymbolsToYAML(UniqueSymbols)` .
Yeah, but we'd put the whole text form of the index into a single in-memory
string, which seems pretty wasteful. Building a YAML output for each seems
silly too though.
Changed `SymbolsToYAML` to take a `raw_ostream` ref parameter to write to.
================
Comment at: clangd/index/Merge.cpp:93
S.Detail = Scratch;
} else if (L.Detail)
+ /* Current state: S.Detail = O.Detail */;
----------------
ioeric wrote:
> `else if (S.Detail)`?
>
> `/*Current state: S.Detail = S.Detail*/`?
Good catch.
Reworked the structure here to avoid needing to name this case, since it was
awkward.
================
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);
----------------
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.
================
Comment at: clangd/index/SymbolCollector.cpp:236
+ std::string FileURI;
+ // FIXME: we may want a different "canonical" heuristic than clang chooses.
+ if (auto DeclLoc = getSymbolLocation(ND, SM, Opts, FileURI))
----------------
ioeric wrote:
> Could you elaborate a bit more on this one? What is the current heuristic,
> and what would we prefer?
Done a bit, though I don't know the detailed answer to either question.
Using forward declarations of classes as canonical indicates that we probably
want something better.
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:108
+ "-std=c++11",
+ "-include",
+ llvm::sys::path::filename(TestHeaderName),
----------------
ioeric wrote:
> neat!
Indeed :-)
This is needed because otherwise all the offsets in the annotated testcase are
off.
I got it slightly wrong though: need the full path to the header name because
it's not resolved relative to the main file.
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
