[PATCH] D73649: [CodeComplete] Member completion for concept-constrained types.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks!



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4905
+      // A requires(){...} lets us infer members from each requirement.
+      for (const concepts::Requirement *Req : RE->getRequirements()) {
+        if (!Req->isDependent())
----------------
kadircet wrote:
> nit s/concepts::Req../auto
I actually don't think the type is obvious here... before I got closely familiar with the concepts AST, I assumed these were expressions.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4910
+          // Do a full traversal so we get `foo` from `typename T::foo::bar`.
+          QualType AssertedType = TR->getType()->getType();
+          ValidVisitor(this, T).TraverseType(AssertedType);
----------------
kadircet wrote:
> TR->getType might fail if there was a substitution failure. check for it before ?
> 
> if(TR->isSubstitutionFailure()) continue;
substitution failures aren't dependent, so we already bailed out in that case. Added a comment.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4984
+        if (Q && isApprox(Q->getAsType(), T))
+          addType(NNS->getAsIdentifier());
+      }
----------------
kadircet wrote:
> as T is dependent i suppose NNS should always be an identifier, but would be nice to spell it out explicitly with a comment.
It doesn't need to be an identifier - it returns null and addType handles null.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5007
+      if (/*Inserted*/ R.second ||
+          std::make_tuple(M.Operator, M.ArgTypes.hasValue(),
+                          M.ResultType != nullptr) >
----------------
kadircet wrote:
> so we'll give up result in case of (syntax might be wrong):
> ```
> ... concept C requires { T::foo; { t.foo() }-> bar }
> ```
> 
> assuming we first traversed t.foo and then T::foo ? I would rather move operator to the end.
Done. This won't ever happen, though.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5077
+  // variables associated with DC (as returned by getTemplatedEntity()).
+  static ::SmallVector<const Expr *, 1>
+  constraintsForTemplatedEntity(DeclContext *DC) {
----------------
kadircet wrote:
> s/::SmallVector/llvm::SmallVector
What the heck?

(preferred spelling seems to be SmallVector here)


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5169
+      } else if (BaseType->isObjCObjectPointerType() ||
+                 BaseType->isTemplateTypeParmType())
         /*Do nothing*/;
----------------
kadircet wrote:
> nit:
> ```
> // ObjcPointers(properties) and TTPT(concepts) are handled below, bail out for the resst.
> else if (!objcPointer && ! TTPT) return false;
> ```
I actually find the 3 cases much harder to spot here, and there's no nice place to put the comment.
Added more braces and extended the comment, WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73649/new/

https://reviews.llvm.org/D73649