ChuanqiXu added a comment. Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM to me basically. Only comments for style left.
================ Comment at: clang/lib/Sema/SemaConcept.cpp:70 + assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?"); + assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?"); + // We should just be able to 'normalize' these to the builtin Binary ---------------- I feel like the usage of the API could be further simplified. ================ Comment at: clang/lib/Sema/SemaConcept.cpp:338-340 + // Backfill the 'converted' list with nulls so we can keep the Converted + Converted.append(ConstraintExprs.size() - Converted.size(), nullptr); + // and unconverted lists in sync. [temp.constr.op] p2 ---------------- ================ Comment at: clang/lib/Sema/SemaConcept.cpp:365-370 + for (unsigned Depth = 0; Depth < TemplateArgsList.getNumSubstitutedLevels(); + ++Depth) { + for (unsigned Index = 0; + Index < TemplateArgsList.getNumSubstitutedArgs(Depth); ++Index) + FlattenedArgs.push_back(TemplateArgsList(Depth, Index)); + } ---------------- I think it is a chance to add range style interfaces for MultiLevelTemplateArgumentList. So that we could use range-based loop here. ================ Comment at: clang/lib/Sema/SemaConcept.cpp:432-476 + if (TemplateArgs) { + MultiLevelTemplateArgumentList JustTemplArgs( + *FD->getTemplateSpecializationArgs()); + if (addInstantiatedParametersToScope( + FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs)) + return true; + } ---------------- The suggested change works. I feel it is not identical with the original. Is it correct or do we miss something in the test? ================ Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2173 Function->setInstantiationOfMemberFunction(D, TSK_ImplicitInstantiation); + } else if (!isFriend) { + // If this is not a function template, and this is not a friend (that is, ---------------- The check here is not straightforward. Here we want to check if the Function is local declared function. But what we check here is about friendness. I am not 100% sure if it is right and other reader might be confusing too. I would suggest to check it directly. e.g, check if one of its DeclContext is FunctionDecl. ================ Comment at: clang/lib/Sema/TreeTransform.h:13003 if (Expr *TRC = E->getCallOperator()->getTrailingRequiresClause()) - // FIXME: Concepts: Substitution into requires clause should only happen - // when checking satisfaction. - NewTrailingRequiresClause = getDerived().TransformExpr(TRC); + NewTrailingRequiresClause = TRC; ---------------- I think we could eliminate `NewTrailingRequiresClause` ================ Comment at: clang/test/SemaTemplate/concepts.cpp:275-276 +template <typename T> +constexpr bool constraint = PR54443::is_same<typename remove_ref<T>::type, + int>::value; + ---------------- Better to rename `constraint` to something like `IsInt` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits