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:
> > > 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.
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
