mizvekov added inline comments.
================ Comment at: clang/lib/Sema/SemaConcept.cpp:744 ArrayRef<const Expr *> E) { - assert(E.size() != 0); - auto First = fromConstraintExpr(S, D, E[0]); ---------------- rsmith wrote: > Might be useful to keep this assert; it will make it clearer to future > readers that this function isn't intended to handle this case. Yeah you are right, the assert makes it obvious this is intended. ================ Comment at: clang/lib/Sema/SemaConcept.cpp:751 + return None; + Conjunction.emplace(S.Context, std::move(*Conjunction), std::move(*Next), + CCK_Conjunction); ---------------- rsmith wrote: > Doesn't this have a use-after-destruction bug? I'd expect `emplace` to first > destroy its contained value and then construct a new one; in this case the > second constructor argument looks like it'll be a reference to a destroyed > object. I assume that's why the old code did the two-step construct and > assign dance. The constructor used here takes both constraints by value: ``` NormalizedConstraint(ASTContext &C, NormalizedConstraint LHS, NormalizedConstraint RHS, CompoundConstraintKind Kind) : Constraint{CompoundConstraint{ new (C) std::pair<NormalizedConstraint, NormalizedConstraint>{ std::move(LHS), std::move(RHS)}, Kind}} { }; ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106907/new/ https://reviews.llvm.org/D106907 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits