aaron.ballman added a comment.
Wow, I *really* like this improvement, thank you for working on it! This is
going to be especially helpful for things like `static_assert(foo() == 12, "oh
no, foo wasn't 12!");`. Can you also add a release note for this improvement?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1536
+def note_bin_op_evaluates : Note<
+ "%select{left-hand|right-hand}0 side of operator '%1' evaluates to '%2'">;
----------------
Should probably add at least one test that shows the "right-hand" version of
the diagnostic.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16564-16571
+ if (isa<IntegerLiteral>(Op->getRHS()->IgnoreParenImpCasts())) {
+ ExprIsRHS = false;
+ ExprSide = Op->getLHS();
+ } else if (isa<IntegerLiteral>(Op->getLHS()->IgnoreParenImpCasts())) {
+ ExprIsRHS = true;
+ ExprSide = Op->getRHS();
+ } else
----------------
I think we want a little bit more smarts here -- `-2` is not an
`IntegerLiteral`, it's a unary expression whose subexpression is an integer
literal. I think we need to handle unary expressions because it'd be weird for
`foo() == 12` to get helpful information but `foo() == -12` not to.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16579-16580
+
+ if (isa<IntegerLiteral>(ExprSide))
+ return;
+
----------------
Similar issue here for unary expressions; we probably don't want `1 == -12` to
get past this check.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16587-16590
+ if (EvalResult.Val.isInt())
+ EvalResult.Val.getInt().toString(ValueString);
+ else
+ return;
----------------
This same logic seems correct for float, fixed point, and complex (though this
one may need some extra work because of the real and imaginary parts). Any
reason not to handle those as well?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130894/new/
https://reviews.llvm.org/D130894
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
