hokein added a comment.
Thanks for the comments!
================
Comment at: clangd/CodeComplete.cpp:319
+//
+// FIXME: we might want to make Sema code completion smarter on handling
+// unresolved qualified-id completion.
----------------
sammccall wrote:
> 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.
I was talking about the `sema infrastructure ` .
================
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.
----------------
sammccall wrote:
> 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)
My original thought was that this structure is used only for qualified
completion (not unqualified completion).
================
Comment at: clangd/CodeComplete.cpp:334
+ // scopes (namespace) in the resolved qualifier;
+ // * unresolved by Sema, global namespace "::" is the only accessible
scope.
+ //
----------------
sammccall wrote:
> 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.
>
Currently Sema doesn't support it, (that being said the visited contexts for
this case is empty).
================
Comment at: clangd/CodeComplete.cpp:340
+ // "namespace ns {using namespace std;} ns::^" => {"ns", "std"}
+ // "std::vec^" => {"::"} // unresolved "std::"
+ std::vector<std::string> AccessibleScopes;
----------------
sammccall wrote:
> 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!)
This was focusing on qualified completions only, the unqualified sample is
provided separately (below).
Have made it together.
================
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.
----------------
sammccall wrote:
> 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).
unfortunately, it is the second one.
================
Comment at: clangd/CodeComplete.cpp:348
+ std::vector<std::string> Results;
+ for (llvm::StringRef VS : AccessibleScopes) {
+ std::string QueryScope =
----------------
sammccall wrote:
> what is "VS"?
means `visible scope`, renamed it to AS.
================
Comment at: clangd/CodeComplete.cpp:350
+ std::string QueryScope =
+ (normalizeScope(VS) +
+ (UnresolvedQualifier
----------------
sammccall wrote:
> sammccall wrote:
> > 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.
> (that patch landed as rL323000)
Thanks. Done.
================
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 =
----------------
sammccall wrote:
> This seems like too much fiddly logic inlined here.
>
> Can't `getQualifiedScopes(CodeCompletionContext, Sema) -> QualifiedScopes`
> handle both cases here?
Yes, we can, that would simplify the code.
================
Comment at: clangd/CodeComplete.cpp:868
+ }
+ log(Ctx, "Query scopes: [");
+ for (auto &R : Req.Scopes)
----------------
sammccall wrote:
> 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.)
I think the log here is pretty useful for debugging.
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
