From patchwork Mon Apr 30 15:48:04 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 906755 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-476996-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="SoJS6XAe"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 40ZTJ22dv9z9s0W for ; Tue, 1 May 2018 01:40:01 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; q=dns; s= default; b=jnwglA3N6YxprYLkF6CRNlNo1qJOU4EnjzF7s5xxTxvY2eQEKUWpG gXJWZa9bWo+RbwDJUOQDdLquaLSM/FibChVddj8Pk/awabwLyhXq/6X3TkasivHK zQKuU0bFUtLOMIQ+vhVKsOI6wo5qyLwcI8NNo//FTXDe1ZL7A/deNg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; s= default; bh=kMBfN6rE/ByU55Un7335R6vqE2I=; b=SoJS6XAepd0w2TUCUedi yCk5QHN2JYrB0JSmltRoD+LL5Wo5rFRu5NTn9HBsHZyVVLrhUG+6nOrPXlj0g+QB WTNy2/YHHRaLW9BYstzwvt6IaLy2ZS3qmcjnvN5Y6muDlXCp0bncQy1jwbzFT0/O mHWdmQ14HKtWyFSAZvoeXWk= Received: (qmail 114662 invoked by alias); 30 Apr 2018 15:39:53 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 114629 invoked by uid 89); 30 Apr 2018 15:39:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=effective, requiring, confusing, five X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Apr 2018 15:39:48 +0000 Received: from smtp.corp.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 206B2C0C274E; Mon, 30 Apr 2018 15:39:47 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-34.phx2.redhat.com [10.3.112.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 219B030012A5; Mon, 30 Apr 2018 15:39:45 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: Richard Sandiford , David Malcolm Subject: [PATCH] selftest: remove "Yoda ordering" in assertions Date: Mon, 30 Apr 2018 11:48:04 -0400 Message-Id: <1525103284-13916-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <87indjc43i.fsf@linaro.org> References: <87indjc43i.fsf@linaro.org> X-IsSubscribed: yes 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(-) 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