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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 conversions after converting enumerations to their underlying types (eg, `<=>` on `enum E : char` converts the operands first to `char` then to `int`). You could remove the `else` here and make this stuff unconditional, but it's probably better to sidestep the extra work and convert directly to the promoted type of the enum's underlying type.
> Do we still do usual arithmetic conversions if we have two enumerations of the same type?
Formally, yes: "If both operands have the same enumeration type E, the operator yields the result of converting the operands to the underlying type of E and applying <=> to the converted operands."

The recursive application of `<=>` to the converted operands will perform the usual arithmetic conversions.


================
Comment at: lib/Sema/SemaExpr.cpp:9794
+  // other is not, the program is ill-formed.
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+    // TODO: What about bool non-narrowing cases? Like '0' or '1.
----------------
You could simplify this to `if (LHSType->isBooleanType() != RHSType->isBooleanType()) InvalidOperands`.


================
Comment at: lib/Sema/SemaExpr.cpp:9795
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+    // TODO: What about bool non-narrowing cases? Like '0' or '1.
+    if (Count != 2) {
----------------
Missing `'`. Seems like a question to take to Herb and CWG (and maybe EWG), but I suspect the answer will be "this does what we wanted".

Looks like P0946R0 missed this case of a difference between `<=>` and other operators... oops.


================
Comment at: lib/Sema/SemaExpr.cpp:9829-9832
+    if (EDecl->isScoped()) {
+      S.InvalidOperands(Loc, LHS, RHS);
+      return QualType();
+    }
----------------
I don't think you need this check: the usual arithmetic conversions will fail to find a common type if the enum is scoped.


================
Comment at: lib/Sema/SemaExpr.cpp:9881-9890
   enum { StrongEquality, PartialOrdering, StrongOrdering } Ordering;
   if (Type->isAnyComplexType())
     Ordering = StrongEquality;
   else if (Type->isFloatingType())
     Ordering = PartialOrdering;
   else
     Ordering = StrongOrdering;
----------------
This is now over-complicated, since this is unreachable for `BO_Cmp`.


================
Comment at: lib/Sema/SemaExpr.cpp:9906
+  bool IsThreeWay = Opc == BO_Cmp;
+  auto IsPointerType = [](ExprResult E) {
+    QualType Ty = E.get()->getType().getNonReferenceType();
----------------
I'd prefer a name that captures that this also recognizes member pointer types.


================
Comment at: lib/Sema/SemaExpr.cpp:9907
+  auto IsPointerType = [](ExprResult E) {
+    QualType Ty = E.get()->getType().getNonReferenceType();
+    return Ty->isPointerType() || Ty->isMemberPointerType();
----------------
You don't need a `.getNonReferenceType()` here; expressions can't have reference type.


https://reviews.llvm.org/D45476