[PATCH 0/2] Improve constraints on gdb::array_view::array_view

[PATCH 0/2] Improve constraints on gdb::array_view::array_view

[Lancelot SIX]

Hi,

Wile reading around, I realized that the gdb::array_view interface has a
potential of being miss-used.  No worry, it is not miss-used in practice
in the current codebase, but fixing this might save debugging time in
the future.

It is possible to create a gdb::array_view<T> to refer to data inside a
std::vector<U> (or any contiguous container of U) if a U* is convertible
to a T*.  If U is a class that inherits from T this condition holds but
U and T most likely do not have the same memory layout. In such
situation the array_view will get it wrong (the second patch gives an
example of a miss-use of a gdb::array_view).

In this series I propose to change slightly the requirements on the
gdb::array_view::array_view constructor so this scenario cannot
compile.  No change is expected from this series.

The first patch just adds a const version of gdb_argv::as_array_view that
will be used in the second patch.  The second patch implements the
change on the gdb::array_view and makes minor adjustments so everything
still compiles.

All feedback are welcome.

Best,
Lancelot.

P.S.  Since I started this series, Simon Marchi have sent another series
dealing with array_view[1].  I have applied this series on top of mine
and check that everything still compiles.

[1] https://sourceware.org/pipermail/gdb-patches/2021-October/182657.html

Lancelot SIX (2):
  Add a const version of gdb_argv:as_array_view
  Improve gdb::array_view ctor from contiguous containers

 gdb/dwarf2/read.c       |  2 +-
 gdb/maint.c             |  2 +-
 gdb/utils.h             | 10 ++++++++++
 gdbsupport/array-view.h |  7 ++++---
 4 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.33.0

[PATCH 1/2] Add a const version of gdb_argv:as_array_view

[Lancelot SIX]

This commit adds a const versions for the gdb_argv::get and the
gdb_argv::as_array_view methods.  Those methods will be used in the
following patch.
---
 gdb/utils.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdb/utils.h b/gdb/utils.h
index f05e6627dca..6f3a70213a4 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -195,6 +195,11 @@ class gdb_argv
     return m_argv;
   }
 
+  const char * const * get () const
+  {
+    return m_argv;
+  }
+
   /* Return the underlying array, transferring ownership to the
      caller.  */
 
@@ -227,6 +232,11 @@ class gdb_argv
     return gdb::array_view<char *> (this->get (), this->count ());
   }
 
+  gdb::array_view<const char * const> as_array_view () const
+  {
+    return gdb::array_view<const char * const> (this->get (), this->count ());
+  }
+
   /* Append arguments to this array.  */
   void append (gdb_argv &&other)
   {
-- 
2.33.0

[PATCH 2/2] Improve gdb::array_view ctor from contiguous containers

[Lancelot SIX]

While reading the interface of gdb::array_view, I realized that the
constructor that builds an array_view on top of a contiguous container
(such as std::vector, std::array or even gdb::array_view) can be
miss-used.

Lets consider the following code sample:

	struct Parent
	{
	  Parent (int a): a { a } {}
	  int a;
	};

	struct Child : public Parent
	{
	  Child (int a, int b): Parent { a }, b { b } {}
	  int b;
	};

	std::ostream &operator<< (std::ostream& os, const Parent & p)
	{ os << "Parent {a=" << p.a << "}"; return os; }
	std::ostream &operator<< (std::ostream& os, const Child & p)
	{ os << "Child {a=" << p.a << ", b=" << p.b << "}"; return os; }

	template <typename T>
	void print_view (const gdb::array_view<const T> &p)
	{
	  std::for_each (p.begin (), p.end (), [](const T &e) { std::cout << e << '\n'; });
	}

Then with the current version of the interface we can write something
like:

	const std::array<Child, 3> elts = {
	  Child {1, 2},
	  Child {3, 4},
	  Child {5, 6}
	};
	print_view<Parent> (elts);

This compiles fine and produces the following output:

	Parent {a=1}
	Parent {a=2}
	Parent {a=3}

This is obviously wrong.  There is nowhere in memory a Parent-like
object for which the A member is 2.  This call to print_all<Parent>
should not compile at all (a call to print_all<Child> instead would
however be fine).

This comes down to the fact that the constructor used in this example
only requires that a Child* is convertible into a Parent*.  This
requirement is valid necessary but not sufficient.

The other constructors of a array_view<T> from data of type U have
stricter requirements on what U should be.  It should satisfy that a U*
can be converted to a T*, but they also require that (decayed) T and
(decayed) U are the same.

This patch proposes to change the constraints on the gdb::array_view
constructor that accepts a Container<U>.  It makes it so the
requirements on what U should be are now the same as for the other
constructors.

Applying this change required minimum adjustment in GDB codebase (two
'const' addition), which are also included in this patch.

Tested by rebuilding using GCC-10.3.0 on GNU/Linux.
---
 gdb/dwarf2/read.c       | 2 +-
 gdb/maint.c             | 2 +-
 gdbsupport/array-view.h | 7 ++++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index cbd9a3012eb..0b82f1bbad4 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14934,7 +14934,7 @@ add_variant_property (struct field_info *fip, struct type *type,
     offset_map[fip->fields[i].offset] = i;
 
   struct objfile *objfile = cu->per_objfile->objfile;
-  gdb::array_view<variant_part> parts
+  gdb::array_view<const variant_part> parts
     = create_variant_parts (&objfile->objfile_obstack, offset_map, fip,
 			    fip->variant_parts);
 
diff --git a/gdb/maint.c b/gdb/maint.c
index 8aae53bdd65..a5e21f458ad 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1128,7 +1128,7 @@ maintenance_selftest (const char *args, int from_tty)
 {
 #if GDB_SELF_TEST
   bool verbose = args != nullptr && check_for_argument (&args, "-verbose");
-  gdb_argv argv (args);
+  const gdb_argv argv (args);
   selftests::run_tests (argv.as_array_view (), verbose);
 #else
   printf_filtered (_("\
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index c41fd620f83..565ad988974 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -139,9 +139,10 @@ class array_view
   template<typename Container,
 	   typename = Requires<gdb::Not<IsDecayedT<Container>>>,
 	   typename
-	     = Requires<std::is_convertible
-			<decltype (std::declval<Container> ().data ()),
-			 T *>>,
+	     = Requires<DecayedConvertible
+			<typename std::remove_pointer
+			 <decltype (std::declval<Container> ().data ())
+			 >::type>>,
 	   typename
 	     = Requires<std::is_convertible
 			<decltype (std::declval<Container> ().size ()),
-- 
2.33.0

Re: [PATCH 1/2] Add a const version of gdb_argv:as_array_view

[Simon Marchi]

On 2021-10-20 16:05, Lancelot SIX via Gdb-patches wrote:
> This commit adds a const versions for the gdb_argv::get and the
> gdb_argv::as_array_view methods.  Those methods will be used in the
> following patch.
> ---
>  gdb/utils.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/gdb/utils.h b/gdb/utils.h
> index f05e6627dca..6f3a70213a4 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -195,6 +195,11 @@ class gdb_argv
>      return m_argv;
>    }
>  
> +  const char * const * get () const
> +  {
> +    return m_argv;
> +  }
> +
>    /* Return the underlying array, transferring ownership to the
>       caller.  */
>  
> @@ -227,6 +232,11 @@ class gdb_argv
>      return gdb::array_view<char *> (this->get (), this->count ());
>    }
>  
> +  gdb::array_view<const char * const> as_array_view () const
> +  {
> +    return gdb::array_view<const char * const> (this->get (), this->count ());
> +  }
> +
>    /* Append arguments to this array.  */
>    void append (gdb_argv &&other)
>    {
> 

It might not be the goal of this particular patch, but operator[] could
also gain a const version.  And then many callers could be constified,
from:

  gdb_argv argv (args);

to

  const gdb_argv argv (args);

But the patch LGTM as-is, that's more for future work.

Simon

Re: [PATCH 2/2] Improve gdb::array_view ctor from contiguous containers

[Simon Marchi]

On 2021-10-20 16:05, Lancelot SIX via Gdb-patches wrote:
> While reading the interface of gdb::array_view, I realized that the
> constructor that builds an array_view on top of a contiguous container
> (such as std::vector, std::array or even gdb::array_view) can be
> miss-used.
> 
> Lets consider the following code sample:
> 
> 	struct Parent
> 	{
> 	  Parent (int a): a { a } {}
> 	  int a;
> 	};
> 
> 	struct Child : public Parent
> 	{
> 	  Child (int a, int b): Parent { a }, b { b } {}
> 	  int b;
> 	};
> 
> 	std::ostream &operator<< (std::ostream& os, const Parent & p)
> 	{ os << "Parent {a=" << p.a << "}"; return os; }
> 	std::ostream &operator<< (std::ostream& os, const Child & p)
> 	{ os << "Child {a=" << p.a << ", b=" << p.b << "}"; return os; }
> 
> 	template <typename T>
> 	void print_view (const gdb::array_view<const T> &p)
> 	{
> 	  std::for_each (p.begin (), p.end (), [](const T &e) { std::cout << e << '\n'; });
> 	}
> 
> Then with the current version of the interface we can write something
> like:
> 
> 	const std::array<Child, 3> elts = {
> 	  Child {1, 2},
> 	  Child {3, 4},
> 	  Child {5, 6}
> 	};
> 	print_view<Parent> (elts);
> 
> This compiles fine and produces the following output:
> 
> 	Parent {a=1}
> 	Parent {a=2}
> 	Parent {a=3}
> 
> This is obviously wrong.  There is nowhere in memory a Parent-like
> object for which the A member is 2.  This call to print_all<Parent>
> should not compile at all (a call to print_all<Child> instead would
> however be fine).
> 
> This comes down to the fact that the constructor used in this example
> only requires that a Child* is convertible into a Parent*.  This
> requirement is valid necessary but not sufficient.
> 
> The other constructors of a array_view<T> from data of type U have
> stricter requirements on what U should be.  It should satisfy that a U*
> can be converted to a T*, but they also require that (decayed) T and
> (decayed) U are the same.
> 
> This patch proposes to change the constraints on the gdb::array_view
> constructor that accepts a Container<U>.  It makes it so the
> requirements on what U should be are now the same as for the other
> constructors.
> 
> Applying this change required minimum adjustment in GDB codebase (two
> 'const' addition), which are also included in this patch.
> 
> Tested by rebuilding using GCC-10.3.0 on GNU/Linux.

The template part is beyond my understanding, but the explanation makes
total sense.  Let's leave a bit of time for others that understand this
more to respond, but if you don't hear anything in ~1 week, then go
ahead and push it.

Simon

Re: [PATCH 2/2] Improve gdb::array_view ctor from contiguous containers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> Tested by rebuilding using GCC-10.3.0 on GNU/Linux.

Simon> The template part is beyond my understanding, but the explanation makes
Simon> total sense.  Let's leave a bit of time for others that understand this
Simon> more to respond, but if you don't hear anything in ~1 week, then go
Simon> ahead and push it.

I think it looks good.
Thanks.

Tom

Re: [PATCH 2/2] Improve gdb::array_view ctor from contiguous containers

[Pedro Alves]

Did you look into covering this scenario in unittests/array-view-selftests.c ?

Re: [PATCH 2/2] Improve gdb::array_view ctor from contiguous containers

[Lancelot SIX]

Hi,

> Did you look into covering this scenario in unittests/array-view-selftests.c ?

I did not.  I did not check for array-view tests in unittests, my bad.
I’ll look into it shortly and come with a testcase.

Lancelot.

[PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers

[Lancelot SIX]

Hi,

Based on Pedro's comment, here is an updated version of the patch with
an added test in array-view-selftests.c.  The first patch of the series
is unchanged.

Best,
Lancelot.

Since V1:

- Add test in gdb/unittests/array-view-selftests.c.
- Rebased onto current trunk and resolved conflicts.

---

While reading the interface of gdb::array_view, I realized that the
constructor that builds an array_view on top of a contiguous container
(such as std::vector, std::array or even gdb::array_view) can be
missused.

Lets consider the following code sample:

	struct Parent
	{
	  Parent (int a): a { a } {}
	  int a;
	};

	std::ostream &operator<< (std::ostream& os, const Parent & p)
	{ os << "Parent {a=" << p.a << "}"; return os; }

	struct Child : public Parent
	{
	  Child (int a, int b): Parent { a }, b { b } {}
	  int b;
	};

	std::ostream &operator<< (std::ostream& os, const Child & p)
	{ os << "Child {a=" << p.a << ", b=" << p.b << "}"; return os; }

	template <typename T>
	void print (const gdb::array_view<const T> &p)
	{
	  std::for_each (p.begin (), p.end (), [](const T &p) { std::cout << p << '\n'; });
	}

Then with the current interface nothinng prevents this usage of
array_view to be done:

	const std::array<Child, 3> elts = {
	  Child {1, 2},
	  Child {3, 4},
	  Child {5, 6}
	};
	print_all<Parent> (elts);

This compiles fine and produces the following output:

	Parent {a=1}
	Parent {a=2}
	Parent {a=3}

which is obviously wrong.  There is nowhere in memory a Parent-like
object for which the A member is 2 and this call to print_all<Parent>
shold not compile at all (calling print_all<Child> is however fine).

This comes down to the fact that a Child* is convertible into a Parent*,
and that an array view is constructed to a pointer to the first element
and a size.  The valid type pointed to that can be used with this
constructor are restricted using SFINAE, which requires that a
pointer to a member into the underlying container can be converted into a
pointer the array_view's data type.

This patch proposes to change the constraints on the gdb::array_view
ctor which accepts a container now requires that the (decayed) type of
the elements in the container match the (decayed) type of the array_view
being constructed.

Applying this change required minimum adjustment in GDB codebase, which
are also included in this patch.

Tested by rebuilding.
---
 gdb/dwarf2/read.c                    |  2 +-
 gdb/maint.c                          |  2 +-
 gdb/unittests/array-view-selftests.c | 21 +++++++++++++++++++++
 gdbsupport/array-view.h              |  7 ++++---
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 48fb55c308c..324b64cb823 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14934,7 +14934,7 @@ add_variant_property (struct field_info *fip, struct type *type,
     offset_map[fip->fields[i].offset] = i;
 
   struct objfile *objfile = cu->per_objfile->objfile;
-  gdb::array_view<variant_part> parts
+  gdb::array_view<const variant_part> parts
     = create_variant_parts (&objfile->objfile_obstack, offset_map, fip,
 			    fip->variant_parts);
 
diff --git a/gdb/maint.c b/gdb/maint.c
index bcc71aab579..e03abfacdae 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1158,7 +1158,7 @@ maintenance_selftest (const char *args, int from_tty)
   auto grp = make_maintenance_selftest_option_group (&opts);
   gdb::option::process_options
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_ERROR, grp);
-  gdb_argv argv (args);
+  const gdb_argv argv (args);
   selftests::run_tests (argv.as_array_view (), opts.verbose);
 #else
   printf_filtered (_("\
diff --git a/gdb/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c
index b62369bc8cd..62013c166db 100644
--- a/gdb/unittests/array-view-selftests.c
+++ b/gdb/unittests/array-view-selftests.c
@@ -21,6 +21,7 @@
 #include "gdbsupport/selftest.h"
 #include "gdbsupport/array-view.h"
 #include <array>
+#include <vector>
 
 namespace selftests {
 namespace array_view_tests {
@@ -116,9 +117,29 @@ check ()
 	  && !is_convertible <C, array_view<B>> ());
 }
 
+/* Check that there is no slicing when build a view from a contiguous
+   container.  */
+
+template<template<typename ...> typename Container>
+static constexpr bool
+check_ctor_from_container ()
+{
+  using gdb::array_view;
+
+  return (    is_convertible <Container<A>, array_view<A>> ()
+	  && !is_convertible <Container<B>, array_view<A>> ()
+	  && !is_convertible <Container<C>, array_view<A>> ()
+
+	  && !is_convertible <Container<A>, array_view<B>> ()
+	  &&  is_convertible <Container<B>, array_view<B>> ()
+	  && !is_convertible <Container<C>, array_view<B>> ());
+}
+
 } /* namespace no_slicing */
 
 static_assert (no_slicing::check (), "");
+static_assert (no_slicing::check_ctor_from_container<std::vector> (), "");
+static_assert (no_slicing::check_ctor_from_container<gdb::array_view> (), "");
 
 /* Check that array_view implicitly converts from std::vector.  */
 
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index ab1d032c60e..6f7e45a979c 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -139,9 +139,10 @@ class array_view
   template<typename Container,
 	   typename = Requires<gdb::Not<IsDecayedT<Container>>>,
 	   typename
-	     = Requires<std::is_convertible
-			<decltype (std::declval<Container> ().data ()),
-			 T *>>,
+	     = Requires<DecayedConvertible
+			<typename std::remove_pointer
+			 <decltype (std::declval<Container> ().data ())
+			 >::type>>,
 	   typename
 	     = Requires<std::is_convertible
 			<decltype (std::declval<Container> ().size ()),
-- 
2.33.0

Re: [PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers

[Pedro Alves]

On 2021-11-03 22:20, Lancelot SIX wrote:
> +/* Check that there is no slicing when build a view from a contiguous
> +   container.  */
> +

The sentence is slightly ungrammatical.

I guess that with "when build a view", you meant either:

  -> "when building a view", or,
  -> "when we build a view"

Alternatively, you could mirror the comment from the built-in array tests:

/* Check that there's no container->view conversion for containers of derived
   types or subclasses.  */


In any case, this is OK.  Thanks for fixing this!

Re: [PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers

[Lancelot SIX]

> Alternatively, you could mirror the comment from the built-in array tests:
> 
> /* Check that there's no container->view conversion for containers of derived
>    types or subclasses.  */
> 
> 
> In any case, this is OK.  Thanks for fixing this!

Hi,

I just pushed this.

Lancelot.

[PATCH] gdb::array_view slicing/container selftest - test std::array too (Re: [PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers)

[Pedro Alves]

On 2021-11-08 23:02, Lancelot SIX via Gdb-patches wrote:
>> Alternatively, you could mirror the comment from the built-in array tests:
>>
>> /* Check that there's no container->view conversion for containers of derived
>>    types or subclasses.  */
>>
>>
>> In any case, this is OK.  Thanks for fixing this!
> 
> Hi,
> 
> I just pushed this.

Thanks.  Looking again, I noticed we don't exercise construction from std::array containers,
unlike other tests.  This adds it.  WDYT?

From 5da7a3deab00d81df9c5fa708520fc05d6a22ffa Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 9 Nov 2021 17:48:50 +0000
Subject: [PATCH] gdb::array_view slicing/container selftest - test std::array
 too

Change-Id: I2141b0b8a09f6521a59908599eb5ba1a19b18dc6
---
 gdb/unittests/array-view-selftests.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c
index 43b7434d10f..fe211a647b5 100644
--- a/gdb/unittests/array-view-selftests.c
+++ b/gdb/unittests/array-view-selftests.c
@@ -137,8 +137,13 @@ check_ctor_from_container ()
 
 } /* namespace no_slicing */
 
+/* std::array with only one template argument, so we can pass it to
+   check_ctor_from_container.  */
+template<typename T> using StdArray1 = std::array<T, 1>;
+
 static_assert (no_slicing::check (), "");
 static_assert (no_slicing::check_ctor_from_container<std::vector> (), "");
+static_assert (no_slicing::check_ctor_from_container<StdArray1> (), "");
 static_assert (no_slicing::check_ctor_from_container<gdb::array_view> (), "");
 
 /* Check that array_view implicitly converts from std::vector.  */

base-commit: f0bbba7886f5dba158a143bebbd0691591f22b9f
-- 
2.26.2

Re: [PATCH] gdb::array_view slicing/container selftest - test std::array too (Re: [PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers)

[Lancelot SIX]

On Tue, Nov 09, 2021 at 05:52:20PM +0000, Pedro Alves wrote:
> On 2021-11-08 23:02, Lancelot SIX via Gdb-patches wrote:
> >> Alternatively, you could mirror the comment from the built-in array tests:
> >>
> >> /* Check that there's no container->view conversion for containers of derived
> >>    types or subclasses.  */
> >>
> >>
> >> In any case, this is OK.  Thanks for fixing this!
> > 
> > Hi,
> > 
> > I just pushed this.
> 
> Thanks.  Looking again, I noticed we don't exercise construction from std::array containers,
> unlike other tests.  This adds it.  WDYT?

Hi,

Actually my first version used a fixed size std::array exactly as you
have here.  I went for a std::vector instead in the version I have
submitted to the list so I still test against one of the standard
containers while not having to define StdArray1.

I am absolutely fine including a test with a std::array. It could even
replace the std::vector based test since this test is the only one that
uses this container (in which case the '#include <vector>' line could
also be dropped).

In the end, both options check that the old erroneous behavior that
used to allow slicing now fails to compile. This is the most important
point.

Best,
Lancelot.

> 
> From 5da7a3deab00d81df9c5fa708520fc05d6a22ffa Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Tue, 9 Nov 2021 17:48:50 +0000
> Subject: [PATCH] gdb::array_view slicing/container selftest - test std::array
>  too
> 
> Change-Id: I2141b0b8a09f6521a59908599eb5ba1a19b18dc6
> ---
>  gdb/unittests/array-view-selftests.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/gdb/unittests/array-view-selftests.c b/gdb/unittests/array-view-selftests.c
> index 43b7434d10f..fe211a647b5 100644
> --- a/gdb/unittests/array-view-selftests.c
> +++ b/gdb/unittests/array-view-selftests.c
> @@ -137,8 +137,13 @@ check_ctor_from_container ()
>  
>  } /* namespace no_slicing */
>  
> +/* std::array with only one template argument, so we can pass it to
> +   check_ctor_from_container.  */
> +template<typename T> using StdArray1 = std::array<T, 1>;
> +
>  static_assert (no_slicing::check (), "");
>  static_assert (no_slicing::check_ctor_from_container<std::vector> (), "");
> +static_assert (no_slicing::check_ctor_from_container<StdArray1> (), "");
>  static_assert (no_slicing::check_ctor_from_container<gdb::array_view> (), "");
>  
>  /* Check that array_view implicitly converts from std::vector.  */
> 
> base-commit: f0bbba7886f5dba158a143bebbd0691591f22b9f
> -- 
> 2.26.2
> 

Re: [PATCH] gdb::array_view slicing/container selftest - test std::array too (Re: [PATCH v2 2/2] Improve gdb::array_view ctor from contiguous containers)

[Pedro Alves]

On 2021-11-09 21:58, Lancelot SIX wrote:
> On Tue, Nov 09, 2021 at 05:52:20PM +0000, Pedro Alves wrote:

>> Thanks.  Looking again, I noticed we don't exercise construction from std::array containers,
>> unlike other tests.  This adds it.  WDYT?
> 
> Hi,
> 
> Actually my first version used a fixed size std::array exactly as you
> have here.  I went for a std::vector instead in the version I have
> submitted to the list so I still test against one of the standard
> containers while not having to define StdArray1.
> 
> I am absolutely fine including a test with a std::array. It could even
> replace the std::vector based test since this test is the only one that
> uses this container (in which case the '#include <vector>' line could
> also be dropped).
> 
> In the end, both options check that the old erroneous behavior that
> used to allow slicing now fails to compile. This is the most important
> point.

The main advantage that I was thinking is that testing more than one
standard container here ensures that we're not baking in some assumption
in the interface (now and forever) that only holds true for std::vector.

I'll merge the patch.

Thanks,
Pedro Alves