EricWF marked 24 inline comments as done.
EricWF added inline comments.
================
Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD,
+ APValue *Dest = nullptr) {
// We don't need to evaluate the initializer for a static local.
----------------
rsmith wrote:
> This `Dest` parameter seems to be unused, is it left behind from a previous
> direction?
Woops. It was, thanks!
================
Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+ return EmitFinalDestCopy(
+ E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}
----------------
rsmith wrote:
> Hm, I wonder whether it's worthwhile to try to generate a select between the
> comparison result values rather than their addresses. (Maybe not, since they
> could in general be an arbitrary aggregate type, and a select on a
> first-class aggregate value is unlikely to produce anything useful at -O0).
I was thinking something similar, but like you said, the STL implementation
could provide an arbitrary aggregate type.
However, I think that's a candidate for a follow up patch, so I'll add a `TODO`
about it and leave it for now.
================
Comment at: lib/Sema/SemaExpr.cpp:9727-9728
+
+static bool checkNarrowingConversion(Sema &S, QualType ToType, Expr *E,
+ QualType FromType, SourceLocation Loc) {
+ // Check for a narrowing implicit conversion.
----------------
rsmith wrote:
> This should have a name that has something to do with three-way comparisons
> (that is, assuming that duplicating this is the best way to customize the
> diagnostic behavior).
I'm not sure this is the cleanest way to do it, but it seems better than trying
to integrate it more directly with the `CheckConvertedConstantExpression`
machinery. The semantics for `operator<=>` seems just different enough.
That being said, I'm very open to suggestions. You're the expert and resident
compiler wizard.
================
Comment at: lib/Sema/SemaExpr.cpp:9807-9810
+ if (!S.Context.hasSameUnqualifiedType(LHSType, RHSType)) {
+ S.InvalidOperands(Loc, LHS, RHS);
+ return QualType();
+ }
----------------
rsmith wrote:
> Please implement the "straight to CWG" resolutions from
> http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18 directly
> here. Specifically, in this case, we should allow three-way comparisons
> between unscoped enumerations and integral types (subject to the narrowing
> check), but not between two unrelated enumeration types, and not between a
> scoped enumeration and an integral type.
I was thinking that if there wasn't already an issue for this behavior, there
should be. Thanks for pointing it out.
================
Comment at: lib/Sema/SemaExpr.cpp:11942
ConvertHalfVec = true;
ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, true);
+ assert(ResultTy.isNull() || ResultTy->getAsCXXRecordDecl());
----------------
rsmith wrote:
> Ah, here it is, `true` is incorrectly being passed for `IsRelational` here.
> Maybe replace that `bool` with an `enum` (or remove it entirely and have the
> callee recompute it from `Opc`)?
Ack. Removing the parameter and re-computing it from `Opc`.
https://reviews.llvm.org/D45476
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
