[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[Sam McCall via Phabricator via cfe-commits]

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