[PATCH] D130894: [clang] Print more information about failed static assertions

[Aaron Ballman via Phabricator via cfe-commits]

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