sammccall added a comment.
This looks great! Would like to get your thoughts on reusing/not reusing
SymbolCollector.
================
Comment at: clangd/FindSymbols.cpp:181
+/// Finds document symbols in the main file of the AST.
+class DocumentSymbolsConsumer : public index::IndexDataConsumer {
+ ASTContext *
----------------
I guess the alternative here is to use SymbolCollector and then convert
Symbol->SymbolInformation (which we should already have for workspace/symbol).
I also guess there's some divergence in behavior, which is why you went this
route :-)
Mostly in filtering? (I'd think that for e.g. printing, we'd want to be
consistent)
Do you think it's worth adding enough customization to SymbolCollector to make
it usable here? Even if it was making `shouldFilterDecl` a std::function,
there'd be some value in unifying the rest. WDYT?
================
Comment at: clangd/SourceCode.cpp:194
+ if (!llvm::sys::path::is_absolute(FilePath)) {
+ if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+ log("Could not turn relative path to absolute: " + FilePath);
----------------
the common case when tryGetRealPathName() is empty seems to be when it's a file
that was part of the preamble we're reusing.
Does this fallback tend to give the same answer in that case? (If so, great! I
know some other places we should reuse this function!)
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:447
+ EXPECT_THAT(getSymbols(FilePath),
+ ElementsAre(AllOf(QName("Tmpl"), WithKind(SymbolKind::Struct)),
+ AllOf(QName("Tmpl::x"), WithKind(SymbolKind::Field)),
----------------
in a perfect world, I think the template definitions might be printed
`Tmpl<T>`, `Tmpl<int>`. Not sure about `Tmpl::x` vs `Tmpl<T>::x` though. WDYT?
(Not necessarily worth doing this patch, keep it simple)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47846
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
