diff mbox series

[gdbsupport] Add gdb::array_view::{iterator,const_iterator}

Message ID 20241012173449.17092-1-tdevries@suse.de
State Superseded
Headers
Series [gdbsupport] Add gdb::array_view::{iterator,const_iterator} |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Oct. 12, 2024, 5:34 p.m. UTC 
  While trying to substitute some std::vector type A in the code with a
gdb::array_view:
...
- using A = std::vector<T>
+ using A = gdb::array_view<T>
....
I ran into the problem that the code was using A::iterator while gdb_array_view
doesn't define such a type.

Fix this by:
- adding types gdb::array_view::iterator and gdb::array_view::const_iterator,
- using them in gdb::array_view::begin and gdb::array_view::end, as is usual,
  and
- using them explicitly in a unit test.

Tested on aarch64-linux.
---
 gdb/unittests/array-view-selftests.c | 36 ++++++++++++++++++++++++++++
 gdbsupport/array-view.h              | 10 ++++----
 2 files changed, 42 insertions(+), 4 deletions(-)


base-commit: f552e90295583446b81a07dcdbad9cd85299ec93

Comments

Tom Tromey Oct. 17, 2024, 7:25 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by:
Tom> - adding types gdb::array_view::iterator and gdb::array_view::const_iterator,
Tom> - using them in gdb::array_view::begin and gdb::array_view::end, as is usual,
Tom>   and
Tom> - using them explicitly in a unit test.

I think it's best to keep this as close to std::span as possible.
See https://sourceware.org/bugzilla/show_bug.cgi?id=31422

Tom> +  constexpr const_iterator begin () const noexcept { return m_array; }

std::span doesn't seem to have this overload but instead calls this
method cbegin.
 
Tom> +  constexpr const_iterator end () const noexcept { return m_array + m_size; }
 
Likewise cend.

Maybe that defeats your plan, I am not sure.
Perhaps we could add / fix up adapters to handle this difference.

Tom
Tom de Vries Oct. 18, 2024, 9:08 a.m. UTC | #2 
On 10/17/24 21:25, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by:
> Tom> - adding types gdb::array_view::iterator and gdb::array_view::const_iterator,
> Tom> - using them in gdb::array_view::begin and gdb::array_view::end, as is usual,
> Tom>   and
> Tom> - using them explicitly in a unit test.
> 

Hi Tom,

thanks for the review.

> I think it's best to keep this as close to std::span as possible.
> See https://sourceware.org/bugzilla/show_bug.cgi?id=31422
> 

I see.

> Tom> +  constexpr const_iterator begin () const noexcept { return m_array; }
> 
> std::span doesn't seem to have this overload but instead calls this
> method cbegin.
>   
> Tom> +  constexpr const_iterator end () const noexcept { return m_array + m_size; }
>   
> Likewise cend.
> 
> Maybe that defeats your plan, I am not sure.

I used the gdb::array_view::iterator in a patch addressing some type 
unit handling problem, but it's on hold because I found more type unit 
related problems, which means that the patch needs revising.  So, I'm 
not sure, but it's not a big deal either.

For now, my goal is to introduce the iterator and const_iterator types, 
which seem to be commonplace in similar classes.

> Perhaps we could add / fix up adapters to handle this difference.

I've written a patch to make the gdb::array_view::{begin,end} interface 
the same as std::span, and rebased this patch on top of it, submitted as 
a v2 series here ( 
https://sourceware.org/pipermail/gdb-patches/2024-October/212458.html ).

I hope that addresses your concerns.

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c
index eb3ae304790..29a8fae3d31 100644
--- a/gdb/unittests/array-view-selftests.c
+++ b/gdb/unittests/array-view-selftests.c
@@ -364,6 +364,39 @@  check_range_for ()
   SELF_CHECK (sum == 1 + 2 + 3 + 4);
 }
 
+template<typename T>
+static void
+check_iterator ()
+{
+  T data[] = {1, 2, 3, 4};
+  gdb::array_view<T> view (data);
+
+  typename std::decay<T>::type sum = 0;
+  for (typename gdb::array_view<T>::iterator it = view.begin ();
+       it != view.end (); it++)
+    {
+      *it *= 2;
+      sum += *it;
+    }
+
+  SELF_CHECK (sum == 2 + 4 + 6 + 8);
+}
+
+template<typename T>
+static void
+check_const_iterator ()
+{
+  T data[] = {1, 2, 3, 4};
+  gdb::array_view<T> view (data);
+
+  typename std::decay<T>::type sum = 0;
+  for (typename gdb::array_view<T>::const_iterator it = view.begin ();
+       it != view.end (); it++)
+    sum += *it;
+
+  SELF_CHECK (sum == 1 + 2 + 3 + 4);
+}
+
 /* Entry point.  */
 
 static void
@@ -490,6 +523,9 @@  run_tests ()
 
   check_range_for<gdb_byte> ();
   check_range_for<const gdb_byte> ();
+  check_iterator<gdb_byte> ();
+  check_const_iterator<gdb_byte> ();
+  check_const_iterator<const gdb_byte> ();
 
   /* Check that the right ctor overloads are taken when the element is
      a container.  */
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index 93842a40ec1..769f0049636 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -88,6 +88,8 @@  class array_view
   using reference = T &;
   using const_reference = const T &;
   using size_type = size_t;
+  using const_iterator = const T *;
+  using iterator = T *;
 
   /* Default construction creates an empty view.  */
   constexpr array_view () noexcept
@@ -157,11 +159,11 @@  class array_view
   constexpr T *data () noexcept { return m_array; }
   constexpr const T *data () const noexcept { return m_array; }
 
-  constexpr T *begin () noexcept { return m_array; }
-  constexpr const T *begin () const noexcept { return m_array; }
+  constexpr iterator begin () noexcept { return m_array; }
+  constexpr const_iterator begin () const noexcept { return m_array; }
 
-  constexpr T *end () noexcept { return m_array + m_size; }
-  constexpr const T *end () const noexcept { return m_array + m_size; }
+  constexpr iterator end () noexcept { return m_array + m_size; }
+  constexpr const_iterator end () const noexcept { return m_array + m_size; }
 
   constexpr reference operator[] (size_t index) noexcept
   {