sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice catch, and nice fix! Might be worth adding a motivating example to the
patch description.
================
Comment at: clangd/index/SymbolCollector.cpp:68
+// For a symbol "a::b::c", return "a::b::". Scope is empty if there's no
+// qualifier. Inline namespaces and unscoped enums are skipped.
+llvm::Expected<std::string> getScope(const NamedDecl *ND) {
----------------
I'd expand on this for inline namespaces a little, because it's non-obvious.
e.g.
// Inline namespaces are treated as transparent scopes.
// This reflects the way they're most commonly used for lookup.
// Ideally we'd include them, but at query time it's hard to find all the
inline
// namespaces to query: the preamble doesn't have a dedicated list.
Conversely, I don't think you need to explicitly mention unscoped enums
anymore, because the behavior for transparent scopes is the obvious one, and we
shouldn't need any special code handling enums (see below suggestions).
================
Comment at: clangd/index/SymbolCollector.cpp:71
+ llvm::SmallVector<llvm::StringRef, 4> Contexts;
+ for (const auto *Context = ND->getDeclContext(); Context;
+ Context = Context->getParent()) {
----------------
if the condition is `!Context->isTranslationUnit()` you can skip the break
inside, which I think reads more naturally. You'll never reach null - only TU
can have a null parent I think.
================
Comment at: clangd/index/SymbolCollector.cpp:71
+ llvm::SmallVector<llvm::StringRef, 4> Contexts;
+ for (const auto *Context = ND->getDeclContext(); Context;
+ Context = Context->getParent()) {
----------------
sammccall wrote:
> if the condition is `!Context->isTranslationUnit()` you can skip the break
> inside, which I think reads more naturally. You'll never reach null - only TU
> can have a null parent I think.
uber-nit: I think `DC` is pretty common in clang to refer to a DeclContext -
once I got used to it, it seems less ambiguous than Context. Up to you though.
================
Comment at: clangd/index/SymbolCollector.cpp:73
+ Context = Context->getParent()) {
+ if (llvm::isa<TranslationUnitDecl>(Context) ||
+ llvm::isa<LinkageSpecDecl>(Context))
----------------
I'm not sure this is always correct: at least clang accepts this code:
namespace X { extern "C++" { int y; }}
and you'll emit "y" instead of "X::y".
I think the check you want is
if (Context->isTransparentContext() || Context->isInlineNamespace())
continue;
isTransparentContext will handle the Namespace and Enum cases as you do below,
including the enum/enum class distinction.
(The code you have below is otherwise correct, I think - but a reader needs to
think about more separate cases in order to see that)
================
Comment at: clangd/index/SymbolCollector.cpp:76
+ break;
+
+ if (const auto *NSD = dyn_cast<NamespaceDecl>(Context)) {
----------------
With the changes suggested above, I think we only get to this point in these
cases:
1. non-inline namespace
2. decl is in some other named scope (class, scoped enum, ...)
Currently case 2 is symbols we're not indexing: shouldFilterDecl() should be
false. So this is a programming error. So I think we just want
Contexts.push_back(cast<NamespaceDecl>(Context)->getName());
which includes an assertion. Returning Expected seems weird here - we should
never hit it unless a precondition is violated.
================
Comment at: clangd/index/SymbolCollector.cpp:90
+ }
+ std::string Scope = llvm::join(Contexts.rbegin(), Contexts.rend(), "::");
+ if (!Scope.empty())
----------------
(nit: might be slightly more obvious just to write the for loop and avoid the
special case, up to you)
================
Comment at: clangd/index/SymbolCollector.cpp:113
// violations.
if (ND->isInAnonymousNamespace())
return true;
----------------
Hmm, if this is ever hot-path, we may want to eventually combine "determine
scope" and "shouldFilter" somewhat. shouldFilterDecl is doing much the same
upwards-scope-traversal, and it seems pretty redundant.
Nothing to do for now though.
================
Comment at: clangd/index/SymbolCollector.cpp:195
llvm::SmallString<128> USR;
+ if (ND->getIdentifier() == nullptr)
+ return true;
----------------
hokein wrote:
> Consider moving to `shouldFilterDecl`? We also have a check `if
> (ND->getDeclName().isEmpty())` there, which I assume does similar thing.
hmm, what case is this handling? should `shouldFilterDecl` catch it?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:326
+TEST_F(SymbolCollectorTest, Scopes) {
+ const std::string Header = R"(
----------------
you could consider modifying one of the testcases to have a weird
linkage-spec-inside-namespace thing I mentioned :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
