[PATCH] D140915: [clangd] Fix getQueryScopes for using-directive with inline namespace

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1709
+
+    // The enclosing namespace must be first, it gets a quality boost.
+    if (auto Enclosing = SpecifiedScopes.EnclosingNamespace) {
----------------
i was actually suggesting to put this logic inside `SpecifiedScope::scopesForIndexQuery` any reason for only including it in this code path?


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1719
+                  });
+    llvm::copy_if(SpecifiedScopes.scopesForQualification(),
+                  std::back_inserter(AccessibleScopes),
----------------
`AccessibleScopes` doesn't need any particular ordering. we can use `scopesForQualification` as-is.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:712
+    std::vector<std::string> EnclosingAtFrontForCompletion;
     std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
+    EnclosingAtFrontForIndex.push_back(EnclosingScope);
----------------
tom-anders wrote:
> kadircet wrote:
> > note that this is actually going to skip inline namespaces (and you're using that in the returned set)
> Hm I should probably fix this and add another regression test for this..?
yeah, let's just leave a fixme.
i also think we should be setting it in all cases, i can't think of a reason for only setting it in a single case. i believe current scope should always get a boost whenever it's part of query scopes.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1660
     // First scope is the (modified) enclosing scope.
     QueryScopes = Scopes.scopesForIndexQuery();
     ScopeProximity.emplace(QueryScopes);
----------------
tom-anders wrote:
> kadircet wrote:
> > we should be setting `AccessibleScopes` too
> Ah thanks, that's what caused the one failing test. I just copied over QueryScopes here for now, looks like this doesn't need any special handling for inline namespaces, does it?
> looks like this doesn't need any special handling for inline namespaces, does it?

well it probably does, but there isn't much we can do. as this code path happens with only heuristic parsing of the main-file code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140915/new/

https://reviews.llvm.org/D140915