[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: include/clang/AST/ASTContext.h:1983 + /// \brief Types and expressions required to build C++2a three-way comparisons + /// using operator<=>, including the values return by builtin <=> operators. + ComparisonCategories

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Fine, that works. https://reviews.llvm.org/D45476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/AST/ExprConstant.cpp:8829 + return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() { +return ExprEvaluatorBaseTy::VisitBinaryOperator(E); + }); rsmith wrote: > EricWF wrote: > > rsmith wrote: > > >

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: EricWF wrote: > EricWF wrote: > > rjmccall wrote: > > > It looks like we don't actually

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: lib/AST/ExprConstant.cpp:8829 + return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() { +return ExprEvaluatorBaseTy::VisitBinaryOperator(E); + }); EricWF wrote: > rsmith wrote: > > It'd be clearer to

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/AST/ExprConstant.cpp:8829 + return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() { +return ExprEvaluatorBaseTy::VisitBinaryOperator(E); + }); rsmith wrote: > It'd be clearer to call `VisitBinCmp`

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: EricWF wrote: > rjmccall wrote: > > It looks like we don't actually support any aggregate

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 8 inline comments as done. EricWF added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:71 + /// standard library. The key is a value of ComparisonCategoryResult. + mutable llvm::DenseMap Objects; + rjmccall

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: It looks like we don't actually support any aggregate types here, which I think is fine

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:71 + /// standard library. The key is a value of ComparisonCategoryResult. + mutable llvm::DenseMap Objects; + EricWF wrote: > rjmccall wrote: > > We expect

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 12 inline comments as done. EricWF added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9825 +LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast); +RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast); +LHSType = RHSType =

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 18 inline comments as done. EricWF added inline comments. Comment at: include/clang/AST/ASTContext.h:1986 + /// This object needs to be initialized by Sema the first time it checks + /// a three-way comparison. + ComparisonCategories CompCategories;

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1986 + /// This object needs to be initialized by Sema the first time it checks + /// a three-way comparison. + ComparisonCategories CompCategories; Is this comment accurate? Because

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked an inline comment as done. EricWF added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:971 + auto EmitCmpRes = [&](const VarDecl *VD) { +return CGF.CGM.GetAddrOfGlobalVar(VD); + }; rsmith wrote: > Perhaps directly emit the constant

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: lib/AST/ExprConstant.cpp:8829 + return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() { +return ExprEvaluatorBaseTy::VisitBinaryOperator(E); + }); It'd be clearer to call `VisitBinCmp` rather than

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1088189, @rsmith wrote: > This is significantly more complexity than we need. We're talking about > constexpr variables here, so we just need a `VarDecl* -- you can then ask > that declaration for its evaluated value and emit that

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1088180, @EricWF wrote: > OK, As I see it, we have two choices: > > (1) Stash the expressions in Sema and import them like > > In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote: > > > I'm sorry, but this is just not true.

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added a comment. This is significantly more complexity than we need. We're talking about constexpr variables here, so we just need a `VarDecl* -- you can then ask that declaration for its evaluated value and emit that directly. https://reviews.llvm.org/D45476

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added a comment. OK, As I see it, we have two choices: (1) Stash the expressions in Sema and import them like In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote: > > Ah. If you want to be able to find this thing without a Sema around in order > to > handle deserialized

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1087487, @EricWF wrote: > In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote: > > > I think you and Richard agreed that you weren’t going to synthesize a whole > > expression tree at every use of the operator, and I agree

Re: [PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

Woops. Submitted that last comment too early. Editing it on Phab. On Fri, May 4, 2018 at 2:31 AM, Eric Fiselier via Phabricator < revi...@reviews.llvm.org> wrote: > EricWF added a comment. > > In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote: > > > I think you and Richard agreed

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added a comment. In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote: > I think you and Richard agreed that you weren’t going to synthesize a whole > expression tree at every use of the operator, and I agree with that > decision. That’s very different from what I’m asking you

Re: [PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

I think you and Richard agreed that you weren’t going to synthesize a whole expression tree at every use of the operator, and I agree with that decision. That’s very different from what I’m asking you to do, which is to synthesize in isolation a call to the copy-constructor. There are several

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

cfe-commits added a subscriber: junbuml. cfe-commits added a comment. I think you and Richard agreed that you weren’t going to synthesize a whole expression tree at every use of the operator, and I agree with that decision. That’s very different from what I’m asking you to do, which is to

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8931 + /*ConstArg*/ true, false, false, false, false); + auto *CopyCtor = cast_or_null(SMI.getMethod()); + rjmccall wrote: > Sorry, I didn't realize you'd go off and

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8931 + /*ConstArg*/ true, false, false, false, false); + auto *CopyCtor = cast_or_null(SMI.getMethod()); + Sorry, I didn't realize you'd go off and write this code

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:1002 + return EmitFinalDestCopy( + E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType())); +} EricWF wrote: > rjmccall wrote: > > Is there something in Sema actually

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:1002 + return EmitFinalDestCopy( + E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType())); +} rjmccall wrote: > Is there something in Sema actually validating that the

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:1002 + return EmitFinalDestCopy( + E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType())); +} Is there something in Sema actually validating that the comparison types is

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9816 +RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast); + } else { +// C++2a [expr.spaceship]p4 EricWF wrote: > rsmith wrote: > > We still need to apply the usual arithmetic

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 3 inline comments as done. EricWF added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9816 +RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast); + } else { +// C++2a [expr.spaceship]p4 rsmith wrote: > We still need to apply the

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9928 +// result is of type std::strong_equality +if (CompositeTy->isFunctionPointerType() || +CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType()) EricWF wrote: >

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 4 inline comments as done. EricWF added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9928 +// result is of type std::strong_equality +if (CompositeTy->isFunctionPointerType() || +CompositeTy->isMemberPointerType() ||

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 24 inline comments as done. EricWF added inline comments. Comment at: lib/AST/ExprConstant.cpp:3784 +static bool EvaluateVarDecl(EvalInfo , const VarDecl *VD, +APValue *Dest = nullptr) { // We don't need to evaluate the initializer

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: lib/Sema/SemaExpr.cpp:9928 +// result is of type std::strong_equality +if (CompositeTy->isFunctionPointerType() || +CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType()) Please add a FIXME

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added a comment. Thanks! This is looking pretty close. Comment at: include/clang/AST/ComparisonCategories.h:78 +public: + /// \brief Wether Sema has fully checked the type and result values for this + /// comparison category types before. Typo

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 30 inline comments as done. EricWF added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:194 + + const ASTContext + mutable llvm::DenseMap Data; Storing the `ASTContext` in

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:77-78 + /// comparison category. For example 'std::strong_equality::equal' + const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const { +const DeclRefExpr *DR =

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:77-78 + /// comparison category. For example 'std::strong_equality::equal' + const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const { +const DeclRefExpr *DR =

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp: +bool Sema::BuildComparisonCategoryData(SourceLocation Loc) { + using CCKT = ComparisonCategoryKind; rsmith wrote: > EricWF wrote: > > rsmith wrote: > > > If you put this on Sema, you'll

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385 +def err_spaceship_comparison_of_void_ptr : Error< + "three-way comparison with void pointer %select{operand type|operand types}0 " + "(%1 and %2)">; +def

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:77-78 + /// comparison category. For example 'std::strong_equality::equal' + const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const { +const DeclRefExpr *DR =

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 7 inline comments as done. EricWF added inline comments. Comment at: lib/AST/ExprConstant.cpp:6238-6263 +bool VisitBinCmp(const BinaryOperator *E) { + using CCR = ComparisonCategoryResult; + const ComparisonCategoryInfo = +

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 5 inline comments as done. EricWF added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377-9378 +def err_implied_comparison_category_type_not_found : Error< + "%0 type was not found; include before defining " + "or using

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 4 inline comments as done. EricWF added inline comments. Comment at: include/clang/AST/Expr.h:3097-3106 + bool isRelationalOp() const { +return isRelationalOp(getOpcode()) || + (getOpcode() == BO_Cmp && IsCmpOrdered); + } static bool

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF marked 4 inline comments as done. EricWF added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:68 + /// \brief A map containing the comparison category values built from the + /// standard library. The key is a value of ComparisonCategoryValue. +

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rsmith added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:68 + /// \brief A map containing the comparison category values built from the + /// standard library. The key is a value of ComparisonCategoryValue. + llvm::DenseMap

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

EricWF added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:155-158 + const ComparisonCategoryInfo (ComparisonCategoryKind Kind) const { +assert(HasData && "comparison category information not yet built"); +return Data[static_cast(Kind)]; + }

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

aaron.ballman added a comment. Thank you for working on this! Comment at: include/clang/AST/ComparisonCategories.h:78 + const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const { +const auto *DR = getResultValueUnsafe(ValueKind); +assert(DR &&