From patchwork Sat Apr 7 18:33:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 26641 Received: (qmail 60412 invoked by alias); 7 Apr 2018 18:34:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 60398 invoked by uid 89); 7 Apr 2018 18:34:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 07 Apr 2018 18:33:59 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0DA8843075E9; Sat, 7 Apr 2018 18:33:58 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 567121102E2B; Sat, 7 Apr 2018 18:33:57 +0000 (UTC) Subject: Re: [PATCH] Add selftests for range_contains and insert_into_bit_range_vector To: Simon Marchi , gdb-patches@sourceware.org References: <20180407143801.19596-1-simon.marchi@polymtl.ca> From: Pedro Alves Message-ID: Date: Sat, 7 Apr 2018 19:33:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180407143801.19596-1-simon.marchi@polymtl.ca> Hi Simon, On 04/07/2018 03:38 PM, Simon Marchi wrote: > I had a patch that converted struct value's VEC into std::vector (as was > done recently by Tom), and had added some selftests to make sure I > didn't mess things up. I thought that since they're already written I > might as well post them. More unit tests good. > /* Definition of a user function. */ > struct internal_function > @@ -3903,6 +3904,131 @@ isvoid_internal_fn (struct gdbarch *gdbarch, > return value_from_longest (builtin_type (gdbarch)->builtin_int, ret); > } > > +namespace selftests > +{ > +static void test_range_contains (void) In non-release builds, I think this will cause a build warning, because this is a static function that is defined but not used then. I.e., this should be wrapped in #if GDB_SELF_TEST too. (see below) Also, line break after first void, and the second void is not necessary. (see below.) Maybe add a comment too? > +{ > + std::vector ranges; > + range r; > + > + /* [10, 14] */ > + r.offset = 10; > + r.length = 5; > + ranges.push_back (r); > + > + /* [20, 24] */ > + r.offset = 20; > + r.length = 5; > + ranges.push_back (r); > + > + /* [2, 6] */ > + SELF_CHECK (!ranges_contain (ranges, 2, 5)); > + /* [9, 13] */ > + SELF_CHECK (ranges_contain (ranges, 9, 5)); > + /* [10, 11] */ > + SELF_CHECK (ranges_contain (ranges, 10, 2)); > + /* [10, 14] */ > + SELF_CHECK (ranges_contain (ranges, 10, 5)); > + /* [13, 18] */ > + SELF_CHECK (ranges_contain (ranges, 13, 6)); > + /* [14, 18] */ > + SELF_CHECK (ranges_contain (ranges, 14, 5)); > + /* [15, 18] */ > + SELF_CHECK (!ranges_contain (ranges, 15, 4)); > + /* [16, 19] */ > + SELF_CHECK (!ranges_contain (ranges, 16, 4)); > + /* [16, 21] */ > + SELF_CHECK (ranges_contain (ranges, 16, 6)); > + /* [21, 21] */ > + SELF_CHECK (ranges_contain (ranges, 21, 1)); > + /* [21, 25] */ > + SELF_CHECK (ranges_contain (ranges, 21, 5)); > + /* [26, 28] */ > + SELF_CHECK (!ranges_contain (ranges, 26, 3)); > +} > + > +/* That that RANGES contains the expected ranges. EXPECTED_N is the expected "Check that" I suppose. > + length of RANGES. The variable arguments are pairs of OFFSET/LENGTH > + corresponding to the expected ranges. */ > + > +static bool check_ranges_vector (const std::vector &ranges, int expected_n, ...) Line too long, and missing break after return type. I think using gdb::array_view here instead of old-style/unsafe varargs would be better. (see more below, and patch at the bottom.) > +{ > + if (expected_n != ranges.size ()) > + return false; > + > + va_list vl; > + va_start (vl, expected_n); > + > + bool ret = true; > + for (const range &r : ranges) > + { > + LONGEST offset = va_arg (vl, LONGEST); > + LONGEST length = va_arg (vl, LONGEST); > + > + if (r.offset != offset || r.length != length) This is calling for range::operator==, IMO. (see below). > + { > + ret = false; > + break; > + } > + } > + > + va_end (vl); > + > + return ret; > +} > + > +static void test_insert_into_bit_range_vector (void) Line break, (void). > +{ > + std::vector ranges; > + > + /* [10, 14] */ > + insert_into_bit_range_vector (&ranges, 10, 5); > + SELF_CHECK (check_ranges_vector (ranges, 1, > + 10, 5)); > + > + /* [10, 14] */ > + insert_into_bit_range_vector (&ranges, 11, 4); > + SELF_CHECK (check_ranges_vector (ranges, 1, > + 10, 5)); > + > + /* [10, 14] [20, 24] */ > + insert_into_bit_range_vector (&ranges, 20, 5); > + SELF_CHECK (check_ranges_vector (ranges, 2, > + 10, 5, > + 20, 5)); > + > + /* [10, 14] [17, 24] */ > + insert_into_bit_range_vector (&ranges, 17, 5); > + SELF_CHECK (check_ranges_vector (ranges, 2, > + 10, 5, > + 17, 8)); > + > + /* [2, 8] [10, 14] [17, 24] */ > + insert_into_bit_range_vector (&ranges, 2, 7); > + SELF_CHECK (check_ranges_vector (ranges, 3, > + 2, 7, > + 10, 5, > + 17, 8)); > + > + /* [2, 14] [17, 24] */ > + insert_into_bit_range_vector (&ranges, 9, 1); > + SELF_CHECK (check_ranges_vector (ranges, 2, > + 2, 13, > + 17, 8)); > + > + /* [2, 14] [17, 24] */ > + insert_into_bit_range_vector (&ranges, 9, 1); > + SELF_CHECK (check_ranges_vector (ranges, 2, > + 2, 13, > + 17, 8)); > + > + /* [2, 33] */ > + insert_into_bit_range_vector (&ranges, 4, 30); > + SELF_CHECK (check_ranges_vector (ranges, 1, > + 2, 32)); Using gdb::array_view avoids having to manually pass the number of expected ranges. > static bool check_ranges_vector (const std::vector &ranges, int expected_n, ...) It can be observed that this function is basically doing a deep comparison of two vectors/arrays of elements of the same type. If the expected ranges were a std::vector too, then you could simplify the function's implementation down to (assuming range::operator==): static bool check_ranges_vector (const std::vector &ranges, const std::vector &expected) { return ranges == expected; } However, since we don't really want to have to heap-allocate the expected vector, we should be able to simply s/std::vector/gdb:array_view/ in the function's parameters, and it should All Just Work. However, it doesn't today, simply because gdb::array_view is currently missing operator==. That's easily fixed, of course. See patch below on top of yours implementing most of the suggestions above, except the missing comments. WDYT? Maybe we could add some operator==/operator!= unit tests to unittests/array-view-selftests.c too. From 7851abdce65332e7adbb383aa0c4219321126100 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Sat, 7 Apr 2018 18:11:04 +0100 Subject: [PATCH] Use array_view instead of varargs --- gdb/common/array-view.h | 27 ++++++++++ gdb/value.c | 138 +++++++++++++++++++++++++++--------------------- 2 files changed, 106 insertions(+), 59 deletions(-) diff --git a/gdb/common/array-view.h b/gdb/common/array-view.h index 3a09ec720cc..319ea99468d 100644 --- a/gdb/common/array-view.h +++ b/gdb/common/array-view.h @@ -174,6 +174,33 @@ private: size_type m_size; }; +/* Compare LHS and RHS for (deep) equality. That is, whether LHS and + RHS have the same sizes, and whether each pair of elements of LHS + and RHS at the same position compares equal. */ + +template +bool +operator== (const gdb::array_view &lhs, const gdb::array_view &rhs) +{ + if (lhs.size () != rhs.size ()) + return false; + + for (size_t i = 0; i < lhs.size (); i++) + if (!(lhs[i] == rhs[i])) + return false; + + return true; +} + +/* Compare two array_views for inequality. */ + +template +bool +operator!= (const gdb::array_view &lhs, const gdb::array_view &rhs) +{ + return !(lhs == rhs); +} + } /* namespace gdb */ #endif diff --git a/gdb/value.c b/gdb/value.c index 0a8531614ff..269928d8d04 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -42,6 +42,7 @@ #include #include "completer.h" #include "selftest.h" +#include "common/array-view.h" /* Definition of a user function. */ struct internal_function @@ -77,6 +78,12 @@ struct range { return offset < other.offset; } + + /* Returns true if THIS is equal to OTHER. */ + bool operator== (const range &other) const + { + return offset == other.offset && length == other.length; + } }; /* Returns true if the ranges defined by [offset1, offset1+len1) and @@ -3904,9 +3911,12 @@ isvoid_internal_fn (struct gdbarch *gdbarch, return value_from_longest (builtin_type (gdbarch)->builtin_int, ret); } +#if GDB_SELF_TEST namespace selftests { -static void test_range_contains (void) + +static void +test_range_contains () { std::vector ranges; range r; @@ -3947,88 +3957,98 @@ static void test_range_contains (void) SELF_CHECK (!ranges_contain (ranges, 26, 3)); } -/* That that RANGES contains the expected ranges. EXPECTED_N is the expected - length of RANGES. The variable arguments are pairs of OFFSET/LENGTH - corresponding to the expected ranges. */ +/* Check that RANGES contains the same ranges as EXPECTED. */ -static bool check_ranges_vector (const std::vector &ranges, int expected_n, ...) +static bool +check_ranges_vector (gdb::array_view ranges, + gdb::array_view expected) { - if (expected_n != ranges.size ()) - return false; - - va_list vl; - va_start (vl, expected_n); - - bool ret = true; - for (const range &r : ranges) - { - LONGEST offset = va_arg (vl, LONGEST); - LONGEST length = va_arg (vl, LONGEST); - - if (r.offset != offset || r.length != length) - { - ret = false; - break; - } - } - - va_end (vl); - - return ret; + return ranges == expected; } -static void test_insert_into_bit_range_vector (void) +static void +test_insert_into_bit_range_vector () { std::vector ranges; /* [10, 14] */ - insert_into_bit_range_vector (&ranges, 10, 5); - SELF_CHECK (check_ranges_vector (ranges, 1, - 10, 5)); + { + insert_into_bit_range_vector (&ranges, 10, 5); + static const range expected[] = { + {10, 5} + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [10, 14] */ - insert_into_bit_range_vector (&ranges, 11, 4); - SELF_CHECK (check_ranges_vector (ranges, 1, - 10, 5)); + { + insert_into_bit_range_vector (&ranges, 11, 4); + static const range expected = {10, 5}; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [10, 14] [20, 24] */ - insert_into_bit_range_vector (&ranges, 20, 5); - SELF_CHECK (check_ranges_vector (ranges, 2, - 10, 5, - 20, 5)); + { + insert_into_bit_range_vector (&ranges, 20, 5); + static const range expected[] = { + {10, 5}, + {20, 5}, + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [10, 14] [17, 24] */ - insert_into_bit_range_vector (&ranges, 17, 5); - SELF_CHECK (check_ranges_vector (ranges, 2, - 10, 5, - 17, 8)); + { + insert_into_bit_range_vector (&ranges, 17, 5); + static const range expected[] = { + {10, 5}, + {17, 8}, + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [2, 8] [10, 14] [17, 24] */ - insert_into_bit_range_vector (&ranges, 2, 7); - SELF_CHECK (check_ranges_vector (ranges, 3, - 2, 7, - 10, 5, - 17, 8)); + { + insert_into_bit_range_vector (&ranges, 2, 7); + static const range expected[] = { + {2, 7}, + {10, 5}, + {17, 8}, + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [2, 14] [17, 24] */ - insert_into_bit_range_vector (&ranges, 9, 1); - SELF_CHECK (check_ranges_vector (ranges, 2, - 2, 13, - 17, 8)); + { + insert_into_bit_range_vector (&ranges, 9, 1); + static const range expected[] = { + {2, 13}, + {17, 8}, + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [2, 14] [17, 24] */ - insert_into_bit_range_vector (&ranges, 9, 1); - SELF_CHECK (check_ranges_vector (ranges, 2, - 2, 13, - 17, 8)); + { + insert_into_bit_range_vector (&ranges, 9, 1); + static const range expected[] = { + {2, 13}, + {17, 8}, + }; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } /* [2, 33] */ - insert_into_bit_range_vector (&ranges, 4, 30); - SELF_CHECK (check_ranges_vector (ranges, 1, - 2, 32)); -} + { + insert_into_bit_range_vector (&ranges, 4, 30); + static const range expected = {2, 32}; + SELF_CHECK (check_ranges_vector (ranges, expected)); + } } +} /* namespace selftests */ +#endif /* GDB_SELF_TEST */ + void _initialize_values (void) {