kadircet 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) + "::");
----------------
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`).
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
