ioeric added inline comments.
================
Comment at: clangd/CodeComplete.cpp:558
+ if (const auto *NS = dyn_cast<NamespaceDecl>(Ctx))
+ return NS->getQualifiedNameAsString() + "::";
+ return llvm::None;
----------------
sammccall wrote:
> does this do the right thing if it's anonymous or inline, or a parent is?
>
> For indexing, we call a function in AST.h, we should probably do something
> similar.
>
> The catch is that function relies on NamedDecl::printQualifiedName, maybe we
> need to factor out the relevant part so we can call it on a DeclContext.
Good catch.
`NamedDecl::printQualifiedName` doesn't skip the `(anonymous)` part if the decl
itself is anonymous, even with `SuppressUnwrittenScope` set. So I think we
should explicitly filter those out here and then call `printQualifiedName`.
================
Comment at: clangd/CodeComplete.cpp:559
+ return NS->getQualifiedNameAsString() + "::";
+ return llvm::None;
+}
----------------
sammccall wrote:
> shouldn't this be ""? That's a definite scope, not a failure to find one.
> (It's not a *namespace* scope, but I'm not sure why that's important)
Make sense.
I think we might still want to special-case the global scope (not here, maybe
in proximity scoring) because:
- if any scope is specified, it's probably more desireable than the global
scope.
- there are patterns like below in cpp files (instead of using enclosing
namespace):
```
using namespace clang;
using namespace clang::clangd;
```
Currently, if the enclosing scope is "", then we would treat it the same as the
scopes from using-namespace directives.
================
Comment at: clangd/Quality.cpp:246
+float QueryScopeProximity::proximity(llvm::StringRef Scope) const {
+ if (QueryScopes.empty())
----------------
sammccall wrote:
> If you're going to decide these numbers directly...
>
> a) I think you should just return a multiplier here, rather than a 0-1 score
> b) we should more aggressively downrank non-preferred symbols: currently by
> only ~1/3 vs symbols from a preferred scopes
>
> e.g. I'd suggest returning 1, 2, 1.5, 1, 1, 1.5, 0.3 or similar
> a) I think you should just return a multiplier here, rather than a 0-1 score.
I tried this. Unfortunately, allowing <1 multiplier would make the default
value (for sema proximity) tricky. And [0,1] proximity score seems to be less
confusing as it's consistent with the file proximity scale. If [0,1] isn't
enough, we could still increase the upper bound?
> b) we should more aggressively downrank non-preferred symbols: currently by
> only ~1/3 vs symbols from a preferred scopes
It's ~1/3 only for the global scope vs non-preferred symbols, which seems
reasonable to me. I worry penalizing non-preferred scopes too much can make
cross-namespace completion less useable.
================
Comment at: clangd/Quality.h:92
+ private:
+ std::vector<std::string> QueryScopes;
+};
----------------
sammccall wrote:
> hmm, can/should we use FileProximity for this, and just transform the strings?
>
> This isn't going to cache for symbols sharing a namespace, isn't going to
> handle "symbol is in a child namespace of an included namespace", and some
> other combinations.
It seems to me that it'd be less trivial to associate scopes with up/down
traverses. Currently, the query scopes contain all enclosing namespaces, so the
distance seems less important here. In general, I think the characteristics of
scope proximity (e.g. all enclosing namespaces are already in query scopes)
allow us to get away with something more trivial than file proximity?
> This isn't going to cache for symbols sharing a namespace.
This seems to be less a concern if we are not calculating up/down distance.
> isn't going to handle "symbol is in a child namespace of an included
> namespace"
This can be covered by `SymbolScope.startswith(QueryScope)`? It might not work
well for `using-namespace`s, but it's unclear how important that is.
Still happy to consider FileDistance-approach as I might be missing something.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
