kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:646
// "" for global namespace, "ns::" for normal namespace.
+ std::vector<std::string> QueryScopes;
+ // Like QueryScopes, but also contains inline namespaces.
----------------
could you actually change the comments above to showcase an inline namespace
and keep this as `AccessibleScopes`.
Then introduce `QueryScopes` with a comment saying that `This is an
overestimate of AccessibleScopes, e.g. it ignores inline namespaces, to fetch
more relevant symbols from index`.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:653
+ // Construct scopes to use for code completion. The results are deduplicated.
+ std::vector<std::string> scopesForCodeCompletion() {
----------------
`Scopes that are accessible from current context. Used for dropping unnecessary
namespecifiers.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:654
+ // Construct scopes to use for code completion. The results are deduplicated.
+ std::vector<std::string> scopesForCodeCompletion() {
+ std::set<std::string> Results;
----------------
s/scopesForCodeCompletion/scopesForQualificaiton
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:676
// (e.g. enclosing namespace).
-std::pair<std::vector<std::string>, bool>
+std::tuple<std::vector<std::string>, std::vector<std::string>, bool>
getQueryScopes(CodeCompletionContext &CCContext, const Sema &CCSema,
----------------
can we have a dedicated struct for return type here? especially the "enclosing
namespace first" logic seem to be getting out of control. so what about
updating `SpecificedScope` to have two new fields `std::optional<std::string>
EnclosingNamespace;` and a `bool AllowAllScopes;`? that way we can just call
getQueryScopes/getAccessibleScopes on needed places and make sure they're
putting the EnclosingNamespace (if it exists) in front.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:712
+ std::vector<std::string> EnclosingAtFrontForCompletion;
std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
+ EnclosingAtFrontForIndex.push_back(EnclosingScope);
----------------
note that this is actually going to skip inline namespaces (and you're using
that in the returned set)
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1660
// First scope is the (modified) enclosing scope.
QueryScopes = Scopes.scopesForIndexQuery();
ScopeProximity.emplace(QueryScopes);
----------------
we should be setting `AccessibleScopes` too
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:674
+ else if (const auto *ND = dyn_cast<NamespaceDecl>(Context)) {
+ if (ND->isInlineNamespace())
+ Scopes.AccessibleScopes.push_back(printQualifiedName(*ND) + "::");
----------------
tom-anders wrote:
> kadircet wrote:
> > tom-anders wrote:
> > > kadircet wrote:
> > > > tom-anders wrote:
> > > > > kadircet wrote:
> > > > > > since we know that the `Context` is a `NamespaceDecl` it should be
> > > > > > safe to use `printQualifiedName` always. any reason for the extra
> > > > > > branching here (apart from minimizing the change to behaviour)? if
> > > > > > not I think we can get rid of the special casing.
> > > > > Unfortunately, this fails CompletionTest.EnclosingScopeComesFirst and
> > > > > CompletionTest.NoDuplicatedQueryScopes, I think because of anonymous
> > > > > namespaces (where printNameSpaceScope would return an empty string,
> > > > > but (printQualifiedName(*ND) + "::" does not).
> > > > i see. taking a closer look at this `getQueryScopes` is used for two
> > > > things:
> > > > - The scopes to query with fuzzyfind requests, hence this should use
> > > > the same "serialization" as symbolcollector (which only strips anon
> > > > namespaces today, but initially it were to strip both anon & inline
> > > > namespaces. it got changed inside clang without clangd tests catching
> > > > it).
> > > > - The shortening of the fully qualified name in
> > > > `CodeCompletionBuilder`. Not having inline namespaces spelled in the
> > > > available namespaces implies getting wrong qualifiers (such as the bug
> > > > you're fixing).
> > > >
> > > > so considering the requirements here:
> > > > - when querying index, we actually want to hide inline namespaces (as
> > > > `ns::hidden::Foo` should be a viable alternative even if only `ns::` is
> > > > accessible). so we should actually fix `printQualifiedName` to set
> > > > `SuppressInlineNamespace` in printing policy to restore the old
> > > > behaviour (and keep using `printNamespaceScope` here).
> > > > - inside `CodeCompletionBuilder`, we shouldn't use the same scopes we
> > > > use during index queries. we should use the visible namespaces while
> > > > preserving inline namespace information and only ignoring the anonymous
> > > > namespaces.
> > > >
> > > > hence can we have 2 separate scopes in `CodeCompleteFlow` instead?
> > > > One called `QueryScopes`, which has the behavior we have today (fixing
> > > > printQualifiedName is a separate issues).
> > > > Other called `AccessibleScopes`, which has accessible namespaces
> > > > spelled **with** inline namespaces, so that we can get proper
> > > > qualification during code-complete.
> > > >
> > > > does that make sense?
> > > tbh I'm a bit confused - I understand your requirements, but am not sure
> > > I understand your proposed solution. Can you expand a bit further?
> > > Looking at the code, there are already both `QueryScopes` and
> > > `AccessibleScopes` variables/fields in various classes, I'm not really
> > > sure at which places you want to make changes.
> > sorry for the long and confusing answer :D
> >
> > I was talking about `CodeCompleteFlow` class specifically, inside
> > `CodeComplete.cpp`. Currently it only has `QueryScopes`, derived from the
> > visible contexts reported by Sema. Unfortunately it loses some granularity
> > to fetch more symbols from index hence it should not be used when picking
> > the required qualifier.
> > My suggestion is to add a new field in `CodeCompleteFlow` called
> > `AccessibleScope`, which is derived at the same stage as `QueryScopes`,
> > while preserving inline namespaces, and used when creating
> > `CodeCompletionBuilder` in `CodeCompleteFlow::toCodeCompletion` (instead of
> > `QueryScopes`).
> Ok, think I got it - To make this a bit less confusing, I first renamed
> SpecifiedScope::AccessibleScopes to SpecifiedScope::QueryScopes (that's the
> number under which it is (and was) stored in CodeCompleteFlow anyway) and
> then added a back a new field AccessibleScopes, that keeps the inline
> namespaces. This is then stored in CodeCompleteFlow:AccessibleScopes and
> passed to the CodeCompletionBuilder instead of QueryScopes.
>
> This now still passes all existing tests and I verified in my editor that it
> also fixes the original issue.
>
> I couldn't figure out how to write a regression test for this now though :(
> I couldn't figure out how to write a regression test for this now though :(
We've got a bunch of tests in `unittests/CodeCompleteTests.cpp`, something like:
```
TEST(CompletionTest, QualificationWithInlineNamespace) {
auto Results = completions(R"cpp(
namespace a { inline namespace b {} }
using namespace a::b;
void f() { Foo^ }
)cpp",
{cls("a::Foo")});
EXPECT_THAT(Results.Completions,
UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo"))));
}
```
should fail without your patch, and pass afterwards.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140915/new/
https://reviews.llvm.org/D140915
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
