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

[Eric Fiselier via Phabricator via cfe-commits]

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:
> > > It'd be clearer to call `VisitBinCmp` rather than `VisitBinaryOperator`.
> > @rsmith: OK, so I'm confused about this. Originally I had an `llvm_unreachable` that the continuation was never reached, but you suggested it was. I'm not sure how. Could you provide an example?
> > 
> > The precondition of calling `VisitBinCmp` is that we have a call to a builtin operator. For `<=>`,  where the composite type is either an arithmetic type, pointer type, or member pointer type (which includes enum types after conversions),  *All* of these cases should be handled before reaching the function.
> > 
> > Is there a control flow path I'm missing? 
> What about comparisons of `_Complex` types, vector types, and any other builtin type that gets added after you commit this patch? The right thing to do (at least for now) in all of those cases is to call the base class implementation, which will deal with emitting the "sorry, I don't know how to constant-evaluate this" diagnostic.
> 
> My comment here was simply that when doing so, you should call the base-class version of the *same* function, which you now do, so that concern is addressed.
Ah, I didn't think about how errors were handled. Thank you.



================
Comment at: lib/CodeGen/CGExprAgg.cpp:964
+    RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+    break;
+  case TEK_Complex:
----------------
rjmccall wrote:
> EricWF wrote:
> > EricWF wrote:
> > > rjmccall wrote:
> > > > It looks like we don't actually support any aggregate types here, which I think is fine because comparing those types is only sensible for things like calls.  If you do want to pave the way for that, or (probably more usefully) for supporting complex types, you should make EmitCompare take the operands as RValues and just use EmitAnyExpr here without paying any attention to the evaluation kind.
> > > Initially I thought the same thing, but apparently member pointers are Aggregates under the Microsoft ABI.
> > > 
> > > I'll give  trafficking in `RValue`s, but all the functions `EmitCompare` calls use `Value*`, so it'll take some work.
> > *I'll give trafficking in `RValue`s a shot, but ...*
> Okay, this would be a *lot* cleaner with RValue.  You can break it down in your EmitCmp helper function instead of EmitCompare if you want, but you've basically just inlined EmitAnyExpr here.
OK, I think I've cleaned it up. Let me know what you think.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:924
+  }();
+  ArgTy->isAnyComplexType();
+  if (ArgTy->hasFloatingRepresentation())
----------------
rjmccall wrote:
> Dead code?
Woops. Removed.


https://reviews.llvm.org/D45476