tom-anders added inline comments.
================
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) + "::");
----------------
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 :(
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
