[PATCH] D42073: [clangd] Use accessible scopes to query indexes for global code completion.

[Haojian Wu via Phabricator via cfe-commits]

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