diff mbox series

selftest: remove "Yoda ordering" in assertions

Message ID 1525103284-13916-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series selftest: remove "Yoda ordering" in assertions | expand

Commit Message

David Malcolm April 30, 2018, 3:48 p.m. UTC 
Our selftest assertions were of the form:

  ASSERT_EQ (expected, actual)

and both Richard Sandiford and I find this "Yoda ordering" confusing.

Our existing tests aren't entirely consistent about this, and it doesn't make
sense for ASSERT_NE and its variants.

The ordering comes from googletest's API, which is what
the earliest version of the selftest code used (before Bernd persuaded
me to stop over-engineering it :) ).

googletest's API now uses just "val1" and "val2" for binary assertion
macros, and their docs now say:

"Historical note: Before February 2016 *_EQ had a convention of calling
it as ASSERT_EQ(expected, actual), so lots of existing code uses this
order. Now *_EQ treats both parameters in the same way."

This seems to have been:
https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb08f3acf
https://github.com/google/googletest/pull/713

This patch renames the params in our selftest API from "expected" and
"actual" to "val1" and "val2".

ASSERT_STREQ (and ASSERT_STREQ_AT) had an asymmetry in error-reporting, where
they did a better job of reporting if the second of the params was NULL; this
patch now handles params equivalently (and both must be non-NULL for a pass).
We aren't able to selftest selftest failures, so I tested the five cases
by hand while developing the patch (4 NULL vs non-NULL cases, with the both
non-NULL case having a pass and fail sub-cases).

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/ChangeLog:
	* selftest.c (assert_streq): Rename "expected" and "actual" to
	"val1" and "val2".  Extend NULL-handling to cover both inputs
	symmetrically, while still requiring both to be non-NULL for a pass.
	* selftest.h (assert_streq): Rename "expected" and "actual" to
	"val1" and "val2".
	(ASSERT_EQ): Likewise.
	(ASSERT_EQ_AT): Likewise.
	(ASSERT_KNOWN_EQ): Likewise.
	(ASSERT_KNOWN_EQ_AT): Likewise.
	(ASSERT_NE): Likewise.
	(ASSERT_MAYBE_NE): Likewise.
	(ASSERT_MAYBE_NE_AT): Likewise.
	(ASSERT_STREQ): Likewise.  Clarify that both must be non-NULL for
	the assertion to pass.
	(ASSERT_STREQ_AT): Likewise.
---
 gcc/selftest.c | 39 ++++++++++++++++++++--------------
 gcc/selftest.h | 66 +++++++++++++++++++++++++++++-----------------------------
 2 files changed, 56 insertions(+), 49 deletions(-)

Comments

Jeff Law April 30, 2018, 10:21 p.m. UTC | #1 
On 04/30/2018 09:48 AM, David Malcolm wrote:
> Our selftest assertions were of the form:
> 
>   ASSERT_EQ (expected, actual)
> 
> and both Richard Sandiford and I find this "Yoda ordering" confusing.
> 
> Our existing tests aren't entirely consistent about this, and it doesn't make
> sense for ASSERT_NE and its variants.
> 
> The ordering comes from googletest's API, which is what
> the earliest version of the selftest code used (before Bernd persuaded
> me to stop over-engineering it :) ).
> 
> googletest's API now uses just "val1" and "val2" for binary assertion
> macros, and their docs now say:
> 
> "Historical note: Before February 2016 *_EQ had a convention of calling
> it as ASSERT_EQ(expected, actual), so lots of existing code uses this
> order. Now *_EQ treats both parameters in the same way."
> 
> This seems to have been:
> https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb08f3acf
> https://github.com/google/googletest/pull/713
> 
> This patch renames the params in our selftest API from "expected" and
> "actual" to "val1" and "val2".
> 
> ASSERT_STREQ (and ASSERT_STREQ_AT) had an asymmetry in error-reporting, where
> they did a better job of reporting if the second of the params was NULL; this
> patch now handles params equivalently (and both must be non-NULL for a pass).
> We aren't able to selftest selftest failures, so I tested the five cases
> by hand while developing the patch (4 NULL vs non-NULL cases, with the both
> non-NULL case having a pass and fail sub-cases).
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/ChangeLog:
> 	* selftest.c (assert_streq): Rename "expected" and "actual" to
> 	"val1" and "val2".  Extend NULL-handling to cover both inputs
> 	symmetrically, while still requiring both to be non-NULL for a pass.
> 	* selftest.h (assert_streq): Rename "expected" and "actual" to
> 	"val1" and "val2".
> 	(ASSERT_EQ): Likewise.
> 	(ASSERT_EQ_AT): Likewise.
> 	(ASSERT_KNOWN_EQ): Likewise.
> 	(ASSERT_KNOWN_EQ_AT): Likewise.
> 	(ASSERT_NE): Likewise.
> 	(ASSERT_MAYBE_NE): Likewise.
> 	(ASSERT_MAYBE_NE_AT): Likewise.
> 	(ASSERT_STREQ): Likewise.  Clarify that both must be non-NULL for
> 	the assertion to pass.
> 	(ASSERT_STREQ_AT): Likewise.
OK.
jeff
diff mbox series

Patch

diff --git a/gcc/selftest.c b/gcc/selftest.c
index 5709110..74adc63 100644
--- a/gcc/selftest.c
+++ b/gcc/selftest.c
@@ -63,27 +63,34 @@  fail_formatted (const location &loc, const char *fmt, ...)
 }
 
 /* Implementation detail of ASSERT_STREQ.
-   Compare val_expected and val_actual with strcmp.  They ought
-   to be non-NULL; fail gracefully if either are NULL.  */
+   Compare val1 and val2 with strcmp.  They ought
+   to be non-NULL; fail gracefully if either or both are NULL.  */
 
 void
 assert_streq (const location &loc,
-	      const char *desc_expected, const char *desc_actual,
-	      const char *val_expected, const char *val_actual)
+	      const char *desc_val1, const char *desc_val2,
+	      const char *val1, const char *val2)
 {
-  /* If val_expected is NULL, the test is buggy.  Fail gracefully.  */
-  if (val_expected == NULL)
-    fail_formatted (loc, "ASSERT_STREQ (%s, %s) expected=NULL",
-		    desc_expected, desc_actual);
-  /* If val_actual is NULL, fail with a custom error message.  */
-  if (val_actual == NULL)
-    fail_formatted (loc, "ASSERT_STREQ (%s, %s) expected=\"%s\" actual=NULL",
-		    desc_expected, desc_actual, val_expected);
-  if (strcmp (val_expected, val_actual) == 0)
-    pass (loc, "ASSERT_STREQ");
+  /* If val1 or val2 are NULL, fail with a custom error message.  */
+  if (val1 == NULL)
+    if (val2 == NULL)
+      fail_formatted (loc, "ASSERT_STREQ (%s, %s) val1=NULL val2=NULL",
+		      desc_val1, desc_val2);
+    else
+      fail_formatted (loc, "ASSERT_STREQ (%s, %s) val1=NULL val2=\"%s\"",
+		      desc_val1, desc_val2, val2);
   else
-    fail_formatted (loc, "ASSERT_STREQ (%s, %s) expected=\"%s\" actual=\"%s\"",
-		    desc_expected, desc_actual, val_expected, val_actual);
+    if (val2 == NULL)
+      fail_formatted (loc, "ASSERT_STREQ (%s, %s) val1=\"%s\" val2=NULL",
+		      desc_val1, desc_val2, val1);
+    else
+      {
+	if (strcmp (val1, val2) == 0)
+	  pass (loc, "ASSERT_STREQ");
+	else
+	  fail_formatted (loc, "ASSERT_STREQ (%s, %s) val1=\"%s\" val2=\"%s\"",
+			  desc_val1, desc_val2, val1, val2);
+      }
 }
 
 /* Implementation detail of ASSERT_STR_CONTAINS.
diff --git a/gcc/selftest.h b/gcc/selftest.h
index fbc2bfe..fc47b2c 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -67,8 +67,8 @@  extern void fail_formatted (const location &loc, const char *fmt, ...)
 /* Implementation detail of ASSERT_STREQ.  */
 
 extern void assert_streq (const location &loc,
-			  const char *desc_expected, const char *desc_actual,
-			  const char *val_expected, const char *val_actual);
+			  const char *desc_val1, const char *desc_val2,
+			  const char *val1, const char *val2);
 
 /* Implementation detail of ASSERT_STR_CONTAINS.  */
 
@@ -263,71 +263,71 @@  extern int num_passes;
     ::selftest::pass ((LOC), desc_);				\
   SELFTEST_END_STMT
 
-/* Evaluate EXPECTED and ACTUAL and compare them with ==, calling
+/* Evaluate VAL1 and VAL2 and compare them with ==, calling
    ::selftest::pass if they are equal,
    ::selftest::fail if they are non-equal.  */
 
-#define ASSERT_EQ(EXPECTED, ACTUAL) \
-  ASSERT_EQ_AT ((SELFTEST_LOCATION), (EXPECTED), (ACTUAL))
+#define ASSERT_EQ(VAL1, VAL2) \
+  ASSERT_EQ_AT ((SELFTEST_LOCATION), (VAL1), (VAL2))
 
 /* Like ASSERT_EQ, but treat LOC as the effective location of the
    selftest.  */
 
-#define ASSERT_EQ_AT(LOC, EXPECTED, ACTUAL)		       \
+#define ASSERT_EQ_AT(LOC, VAL1, VAL2)		       \
   SELFTEST_BEGIN_STMT					       \
-  const char *desc_ = "ASSERT_EQ (" #EXPECTED ", " #ACTUAL ")"; \
-  if ((EXPECTED) == (ACTUAL))				       \
+  const char *desc_ = "ASSERT_EQ (" #VAL1 ", " #VAL2 ")"; \
+  if ((VAL1) == (VAL2))				       \
     ::selftest::pass ((LOC), desc_);			       \
   else							       \
     ::selftest::fail ((LOC), desc_);			       \
   SELFTEST_END_STMT
 
-/* Evaluate EXPECTED and ACTUAL and compare them with known_eq, calling
+/* Evaluate VAL1 and VAL2 and compare them with known_eq, calling
    ::selftest::pass if they are always equal,
    ::selftest::fail if they might be non-equal.  */
 
-#define ASSERT_KNOWN_EQ(EXPECTED, ACTUAL) \
-  ASSERT_KNOWN_EQ_AT ((SELFTEST_LOCATION), (EXPECTED), (ACTUAL))
+#define ASSERT_KNOWN_EQ(VAL1, VAL2) \
+  ASSERT_KNOWN_EQ_AT ((SELFTEST_LOCATION), (VAL1), (VAL2))
 
 /* Like ASSERT_KNOWN_EQ, but treat LOC as the effective location of the
    selftest.  */
 
-#define ASSERT_KNOWN_EQ_AT(LOC, EXPECTED, ACTUAL)			\
+#define ASSERT_KNOWN_EQ_AT(LOC, VAL1, VAL2)			\
   SELFTEST_BEGIN_STMT							\
-  const char *desc = "ASSERT_KNOWN_EQ (" #EXPECTED ", " #ACTUAL ")";	\
-  if (known_eq (EXPECTED, ACTUAL))					\
+  const char *desc = "ASSERT_KNOWN_EQ (" #VAL1 ", " #VAL2 ")";	\
+  if (known_eq (VAL1, VAL2))					\
     ::selftest::pass ((LOC), desc);					\
   else									\
     ::selftest::fail ((LOC), desc);					\
   SELFTEST_END_STMT
 
-/* Evaluate EXPECTED and ACTUAL and compare them with !=, calling
+/* Evaluate VAL1 and VAL2 and compare them with !=, calling
    ::selftest::pass if they are non-equal,
    ::selftest::fail if they are equal.  */
 
-#define ASSERT_NE(EXPECTED, ACTUAL)			       \
+#define ASSERT_NE(VAL1, VAL2)			       \
   SELFTEST_BEGIN_STMT					       \
-  const char *desc_ = "ASSERT_NE (" #EXPECTED ", " #ACTUAL ")"; \
-  if ((EXPECTED) != (ACTUAL))				       \
+  const char *desc_ = "ASSERT_NE (" #VAL1 ", " #VAL2 ")"; \
+  if ((VAL1) != (VAL2))				       \
     ::selftest::pass (SELFTEST_LOCATION, desc_);	       \
   else							       \
     ::selftest::fail (SELFTEST_LOCATION, desc_);	       \
   SELFTEST_END_STMT
 
-/* Evaluate EXPECTED and ACTUAL and compare them with maybe_ne, calling
+/* Evaluate VAL1 and VAL2 and compare them with maybe_ne, calling
    ::selftest::pass if they might be non-equal,
    ::selftest::fail if they are known to be equal.  */
 
-#define ASSERT_MAYBE_NE(EXPECTED, ACTUAL) \
-  ASSERT_MAYBE_NE_AT ((SELFTEST_LOCATION), (EXPECTED), (ACTUAL))
+#define ASSERT_MAYBE_NE(VAL1, VAL2) \
+  ASSERT_MAYBE_NE_AT ((SELFTEST_LOCATION), (VAL1), (VAL2))
 
 /* Like ASSERT_MAYBE_NE, but treat LOC as the effective location of the
    selftest.  */
 
-#define ASSERT_MAYBE_NE_AT(LOC, EXPECTED, ACTUAL)			\
+#define ASSERT_MAYBE_NE_AT(LOC, VAL1, VAL2)			\
   SELFTEST_BEGIN_STMT							\
-  const char *desc = "ASSERT_MAYBE_NE (" #EXPECTED ", " #ACTUAL ")";	\
-  if (maybe_ne (EXPECTED, ACTUAL))					\
+  const char *desc = "ASSERT_MAYBE_NE (" #VAL1 ", " #VAL2 ")";	\
+  if (maybe_ne (VAL1, VAL2))					\
     ::selftest::pass ((LOC), desc);					\
   else									\
     ::selftest::fail ((LOC), desc);					\
@@ -371,23 +371,23 @@  extern int num_passes;
     ::selftest::fail ((LOC), desc_);			       \
   SELFTEST_END_STMT
 
-/* Evaluate EXPECTED and ACTUAL and compare them with strcmp, calling
-   ::selftest::pass if they are equal,
-   ::selftest::fail if they are non-equal.  */
+/* Evaluate VAL1 and VAL2 and compare them with strcmp, calling
+   ::selftest::pass if they are equal (and both are non-NULL),
+   ::selftest::fail if they are non-equal, or are both NULL.  */
 
-#define ASSERT_STREQ(EXPECTED, ACTUAL)				    \
+#define ASSERT_STREQ(VAL1, VAL2)				    \
   SELFTEST_BEGIN_STMT						    \
-  ::selftest::assert_streq (SELFTEST_LOCATION, #EXPECTED, #ACTUAL, \
-			    (EXPECTED), (ACTUAL));		    \
+  ::selftest::assert_streq (SELFTEST_LOCATION, #VAL1, #VAL2, \
+			    (VAL1), (VAL2));		    \
   SELFTEST_END_STMT
 
 /* Like ASSERT_STREQ, but treat LOC as the effective location of the
    selftest.  */
 
-#define ASSERT_STREQ_AT(LOC, EXPECTED, ACTUAL)			    \
+#define ASSERT_STREQ_AT(LOC, VAL1, VAL2)			    \
   SELFTEST_BEGIN_STMT						    \
-  ::selftest::assert_streq ((LOC), #EXPECTED, #ACTUAL,		    \
-			    (EXPECTED), (ACTUAL));		    \
+  ::selftest::assert_streq ((LOC), #VAL1, #VAL2,		    \
+			    (VAL1), (VAL2));		    \
   SELFTEST_END_STMT
 
 /* Evaluate HAYSTACK and NEEDLE and use strstr to determine if NEEDLE