sammccall added a comment.
I think this mostly does the right thing, but I find some of the code
structure/description confusing.
I might have made misguided comments, happy to sit down together and work this
out :-)
================
Comment at: clangd/CodeComplete.cpp:316
+
+/// \brief Scopes being queried in indexes for the qualified-id code completion
+/// (e.g. "ns::ab^").
----------------
This comment is a bit confusing.
If this struct is trying to model "what we're going to query the index for"
then it's not needed - just use vector<string>.
If it's trying to model the context of the identifier we're completing, then
this comment is inaccurate and should be something like `The namespace context
of the partial identifier we're trying to complete.`
Then add something like `We use this when querying the index for more results.`
================
Comment at: clangd/CodeComplete.cpp:316
+
+/// \brief Scopes being queried in indexes for the qualified-id code completion
+/// (e.g. "ns::ab^").
----------------
sammccall wrote:
> This comment is a bit confusing.
> If this struct is trying to model "what we're going to query the index for"
> then it's not needed - just use vector<string>.
> If it's trying to model the context of the identifier we're completing, then
> this comment is inaccurate and should be something like `The namespace
> context of the partial identifier we're trying to complete.`
> Then add something like `We use this when querying the index for more
> results.`
nit: avoid \brief where possible - the first sentence is used by default.
================
Comment at: clangd/CodeComplete.cpp:319
+//
+// FIXME: we might want to make Sema code completion smarter on handling
+// unresolved qualified-id completion.
----------------
it's not totally clear to me whether you're talking about *our* sema-based
results, or the sema infrastructure we're using, or it's a mistake and you mean
our index code completion.
Maybe just:
// FIXME: When we resolve part of a scope chain (e.g.
"known::unknown::ident"), we should
// expand the known part rather than treating the whole thing as unknown.
I think this should go in the implementation, rather than the type comment.
================
Comment at: clangd/CodeComplete.cpp:327-328
+// way is to return an unresolved qualifier "ns1::ns2" with all scopes that are
+// accessible in "foo".
+struct QualifiedScopes {
+ // All scopes that are accessible in the completion scope qualifier.
----------------
Hmm, you're renamed this from `SpecifiedScope` to `QualifiedScope`. I don't
understand this name, can you explain what it means?
(SpecifiedScope refers to the scope that the *user* specified when typing)
================
Comment at: clangd/CodeComplete.cpp:329
+struct QualifiedScopes {
+ // All scopes that are accessible in the completion scope qualifier.
+ //
----------------
This is a bit hard to parse, and doesn't seem to describe the "unresolved by
sema" case in a meaningful way.
What about:
The scopes we should look in, determined by Sema.
If the qualifier was fully resolved, we should look for completions in these
scopes.
If there is an unresolved part of the qualifier, it should be resolved within
these scopes.
That way we describe what we aim to do (which is pretty simple), and we can
document deviations with FIXMEs.
================
Comment at: clangd/CodeComplete.cpp:334
+ // scopes (namespace) in the resolved qualifier;
+ // * unresolved by Sema, global namespace "::" is the only accessible
scope.
+ //
----------------
for this case: why not "the containing namespace and all accessible
namespaces"? You've implemented that below, and it seems like the right thing
to do here.
================
Comment at: clangd/CodeComplete.cpp:340
+ // "namespace ns {using namespace std;} ns::^" => {"ns", "std"}
+ // "std::vec^" => {"::"} // unresolved "std::"
+ std::vector<std::string> AccessibleScopes;
----------------
you seem to have a comment-in-a-comment here.
nit: please line up the => so it forms a table.
I'd like to see an example `"namespace ns { vec^ }" => {"ns", ""}
(I know it's not what the code currently does, but IMO it should!)
================
Comment at: clangd/CodeComplete.cpp:342
+ std::vector<std::string> AccessibleScopes;
+ // The scope qualifier that is not resolved in Sema, it is the user-written
+ // qualifiers.
----------------
This is ambiguous, does it mean `The suffix of the user-writter qualifiers that
Sema didn't resolve` or `The full scope qualifier as typed by the user`?
(I'm hoping the former, but not sure).
================
Comment at: clangd/CodeComplete.cpp:346
+
+ std::vector<std::string> forIndex() {
+ std::vector<std::string> Results;
----------------
this needs a new name. Previously it described one scope, and was being
formatted to match the index representation.
Now it contains logic to determine the index query, so maybe
`scopesForIndexQuery()`?
================
Comment at: clangd/CodeComplete.cpp:348
+ std::vector<std::string> Results;
+ for (llvm::StringRef VS : AccessibleScopes) {
+ std::string QueryScope =
----------------
what is "VS"?
================
Comment at: clangd/CodeComplete.cpp:350
+ std::string QueryScope =
+ (normalizeScope(VS) +
+ (UnresolvedQualifier
----------------
It seems like you have stored the scopes in an unknown format, and call
"normalizeScope" defensively? Seems cleaner if you ensure both the scopes and
unknown are in the form:
- global = ""
- top-level = "::foo"
- nested = "::foo::bar"
Then this code can produce similarly-formatted output with just:
Results.push_back(VS);
if (UnresolvedQualifier)
Results.back() += *UnresolvedQualifier;
We need to trim leading `::` before sending to the index, but I think that's
because we got the index API wrong. I'll send a patch to fix it.
================
Comment at: clangd/CodeComplete.cpp:845
// If the user typed a scope, e.g. a::b::xxx(), restrict to that scope.
- // FIXME(ioeric): add scopes based on using directives and enclosing ns.
- if (auto SS = Recorder.CCContext.getCXXScopeSpecifier())
- Req.Scopes = {getSpecifiedScope(*Recorder.CCSema, **SS).forIndex()};
- else
- // Unless the user typed a ns qualifier, complete in global scope only.
- // FIXME: once we know what namespaces are in scope (D42073), use those.
+ if (auto SS = Recorder.CCContext.getCXXScopeSpecifier()) {
+ Req.Scopes =
----------------
This seems like too much fiddly logic inlined here.
Can't `getQualifiedScopes(CodeCompletionContext, Sema) -> QualifiedScopes`
handle both cases here?
================
Comment at: clangd/CodeComplete.cpp:849
+ .forIndex();
+ } else {
+ // Unqualified code completion. Use the containing namespace and all
----------------
Please use a `QualifiedScopes` object to handle this case too.
It's simple: the unresolved part is "", and the list of accessible scopes is
the same one you're computing here.
================
Comment at: clangd/CodeComplete.cpp:863
+ if (isa<TranslationUnitDecl>(Context))
+ Req.Scopes.push_back(normalizeScope("::")); // global namespace
+ else if (const auto*NS = dyn_cast<NamespaceDecl>(Context))
----------------
the global scope is spelled `""` in both conventions we use (the current one,
and the one we should be using instead :-)).
================
Comment at: clangd/CodeComplete.cpp:868
+ }
+ log(Ctx, "Query scopes: [");
+ for (auto &R : Req.Scopes)
----------------
log doesn't work that way - you need to combine into a single message :-)
I think since we don't have verbosity levels, it's best to remove this - it's
much more fine-grained than anything else we log, so it will be spammy.
(If we do want to log something, we should summarize the whole query.)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42073
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
