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 
> 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`.

cfe-commits mailing list

Reply via email to