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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/CodeComplete.cpp:270
+  /// namespace scopes which are visible to the qualified-id completion token.
+  std::vector<std::string> Scopes;
+};
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > Just to check, if the user types:
> > > > "vec" --> None
> > > > "::vec" --> {""}
> > > > "std::vec" --> {"std"}
> > > > "using namespace std; vec" --> None
> > > > "using namespace std; ::vec" --> {"", "std"}
> > > > 
> > > > is this right?
> > > I think the idea was to have (I only highlight the differences):
> > > "vec" --> {""}
> > > "using namespace std; vec" --> {"", "std"}
> > > 
> > > @hokein , or am I getting it wrong?
> > You're probably right, just want to be sure we're talking about the same thing.
> > 
> > There's two layers here: the context detected from sema, and what we're going to send the index. The layering is more relevant now that more of this moves out of clangd.
> > 
> > for "vec", we should be sending {""} to the index for now, and later move towards doing global completion.
> > But if possible the detection code should report None, and the query-index code should translate it - there's no reason the detection code needs to be wrong just because clangd can't do qualifier insertion/smart ranking/etc.
> > 
> > That said, per our discussion this morning, the detected state shouldn't really be Optional<vector<string>>, but rather struct { vector<string> AccessibleScope, optional<string> UnresolvedQualifier } or something like that...
> - Totally agree, this patch only fills the `vector<string> AccessibleScope` field, and `optional<string> UnresolvedQualifier` could be added in the follow-up patch when it's actually handled.
> - It still makes sense to have `optional<QueryScopes>` to distinguish cases when clang did not run the lookup during completion and the scopes were not set (i.e. completion inside macros, etc.)
@ilya-biryukov your are right. 

Based on our discussion last morning, the accessible scopes:

  //   qualified completion
  //   "::vec" => {"::"}
  //   "using namespace std; ::vec^" => {"::", "std"}
  //   "namespace ns {using namespace std;} ns::^" => {"ns", "std"}
  //   "std::vec^" => {"::"}  // unresolved "std::"
  //
  //   unqualified completion
  //   "vec^" => {"::"}
  //   "using namespace std; vec^" => {"::", "std"}
  //   "using namespace std; namespace ns { vec^ }" => {"ns", "std", "::"}


With rL322945, now it is sensible to include all (qualified/unqualified completion) in this patch.


================
Comment at: clangd/CodeComplete.cpp:658
+    VisibleNamespaceFinder Finder;
+    S.LookupVisibleDecls(QCC.ClosestScope,
+                         clang::Sema::LookupNameKind::LookupOrdinaryName,
----------------
sammccall wrote:
> you need to set loadexternal to false to avoid deserializing the whole preamble.
> Ilya says this is going to get merged with lookup during code-completion, which would be nice :-)
not needed now as we don't do lookup manually in clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42073