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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang/include/clang/Sema/Scope.h:323
   /// declared in.
-  bool isDeclScope(Decl *D) {
-    return DeclsInScope.count(D) != 0;
-  }
+  bool isDeclScope(const Decl *D) { return DeclsInScope.count(D) != 0; }
 
----------------
also mark the member as `const` ?


================
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())
----------------
nit s/concepts::Req../auto


================
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);
----------------
TR->getType might fail if there was a substitution failure. check for it before ?

if(TR->isSubstitutionFailure()) continue;


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


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5007
+      if (/*Inserted*/ R.second ||
+          std::make_tuple(M.Operator, M.ArgTypes.hasValue(),
+                          M.ResultType != nullptr) >
----------------
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.


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


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5094
+
+  static QualType extractExactType(const TypeConstraint &T) {
+    // Assume a same_as<T> return type constraint is std::same_as or equivalent.
----------------
maybe rename this to `deduceType`?

Also some comments explaining that this is a `beautify heuristic` might be nice.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5169
+      } else if (BaseType->isObjCObjectPointerType() ||
+                 BaseType->isTemplateTypeParmType())
         /*Do nothing*/;
----------------
nit:
```
// ObjcPointers(properties) and TTPT(concepts) are handled below, bail out for the resst.
else if (!objcPointer && ! TTPT) return false;
```


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5831
+  NestedNameSpecifier *NNS = SS.getScopeRep();
+  if (SS.isNotEmpty() && SS.isValid() && !NNS->isDependent()) {
+    if (Ctx == nullptr || RequireCompleteDeclContext(SS, Ctx))
----------------
SS.isNotEmpty is same as NNS!=nullptr, maybe replace with that to make it more clear ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73649