This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add selftests for range_contains and insert_into_bit_range_vector


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<range> 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<range> &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<range> 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<range> &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<range> &ranges, 
                      const std::vector<range> &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 <palves@redhat.com>
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 <typename T>
+bool
+operator== (const gdb::array_view<T> &lhs, const gdb::array_view<T> &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 <typename T>
+bool
+operator!= (const gdb::array_view<T> &lhs, const gdb::array_view<T> &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 <algorithm>
 #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<range> 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<range> &ranges, int expected_n, ...)
+static bool
+check_ranges_vector (gdb::array_view<const range> ranges,
+		     gdb::array_view<const range> 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<range> 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)
 {
-- 
2.14.3


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]