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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits