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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
