[v3] PR libstdc++/87619

Message ID CAFk2RUYRRCtoFro2zZzt7YXg4Z02=-+kCzPqTaRywYVDwcu0rQ@mail.gmail.com
State New
Headers show
Series
  • [v3] PR libstdc++/87619
Related show

Commit Message

Ville Voutilainen Oct. 16, 2018, 7:59 p.m.
Simple, short, and sweet, just a thinko.

2018-10-16  Ville Voutilainen  <ville.voutilainen@gmail.com>

    PR libstdc++/87619
    * include/std/variant (__select_index): Fix an off-by-one.
    * testsuite/20_util/variant/87619.cc: New.

Comments

Jonathan Wakely Oct. 17, 2018, 9:07 a.m. | #1
On 16/10/18 22:59 +0300, Ville Voutilainen wrote:
>Simple, short, and sweet, just a thinko.

>

>2018-10-16  Ville Voutilainen  <ville.voutilainen@gmail.com>

>

>    PR libstdc++/87619

>    * include/std/variant (__select_index): Fix an off-by-one.

>    * testsuite/20_util/variant/87619.cc: New.


>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant

>index 2d86a70..55b6440 100644

>--- a/libstdc++-v3/include/std/variant

>+++ b/libstdc++-v3/include/std/variant

>@@ -362,7 +362,7 @@ namespace __variant

> 

>   template <typename... _Types>

>   using __select_index =

>-    typename __select_int::_Select_int_base<sizeof...(_Types) + 1,

>+    typename __select_int::_Select_int_base<sizeof...(_Types),

> 					    unsigned char,

> 					    unsigned short>::type::value_type;

> 

>diff --git a/libstdc++-v3/testsuite/20_util/variant/87619.cc b/libstdc++-v3/testsuite/20_util/variant/87619.cc

>new file mode 100644

>index 0000000..a8db6b8

>--- /dev/null

>+++ b/libstdc++-v3/testsuite/20_util/variant/87619.cc

>@@ -0,0 +1,46 @@

>+// Copyright (C) 2018 Free Software Foundation, Inc.

>+//

>+// This file is part of the GNU ISO C++ Library.  This library is free

>+// software; you can redistribute it and/or modify it under the

>+// terms of the GNU General Public License as published by the

>+// Free Software Foundation; either version 3, or (at your option)

>+// any later version.

>+

>+// This library is distributed in the hope that it will be useful,

>+// but WITHOUT ANY WARRANTY; without even the implied warranty of

>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

>+// GNU General Public License for more details.

>+

>+// You should have received a copy of the GNU General Public License along

>+// with this library; see the file COPYING3.  If not see

>+// <http://www.gnu.org/licenses/>.

>+

>+// { dg-options "-std=gnu++17" }

>+// { dg-do compile { target c++17 } }

>+

>+#include <variant>

>+#include <utility>

>+

>+

>+template<std::size_t I>

>+struct S {

>+};

>+

>+template <std::size_t... Is>

>+void f_impl(std::index_sequence<Is...>)

>+{

>+	using V = std::variant<S<Is>...>;

>+	static_assert(sizeof(V) == 2);


It might be worth also calling f<256>() and doing:

template <std::size_t... Is>
void f_impl(std::index_sequence<Is...> is)
{
  using V = std::variant<S<Is>...>;

  // For a variant of 255 alternatives the valid indices are [0,254]
  // and index 255 means valueless-by-exception, so fits in one byte.
  if constexpr (std::variant_size_v<V> <= 255)
    static_assert(sizeof(V) == 2)
  else
    static_assert(sizeof(V) > 2)
}

Just to check we don't introduce an off-by-one error in the *other*
direction in future. What do you think?

To be really portable we would use numeric_limits<unsigned char>::max()
but we don't need to worry about non-8-bit char in our implementation.

>+}

>+

>+template <std::size_t I>

>+void f()

>+{

>+	f_impl(std::make_index_sequence<I>{});

>+}

>+

>+int main()

>+{

>+	f<254>();

>+	f<255>();

>+}
Ville Voutilainen Oct. 17, 2018, 9:20 a.m. | #2
On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote:
> It might be worth also calling f<256>() and doing:

>

> template <std::size_t... Is>

> void f_impl(std::index_sequence<Is...> is)

> {

>   using V = std::variant<S<Is>...>;

>

>   // For a variant of 255 alternatives the valid indices are [0,254]

>   // and index 255 means valueless-by-exception, so fits in one byte.

>   if constexpr (std::variant_size_v<V> <= 255)

>     static_assert(sizeof(V) == 2)

>   else

>     static_assert(sizeof(V) > 2)

> }

>

> Just to check we don't introduce an off-by-one error in the *other*

> direction in future. What do you think?


Agreed. I'll patch that in.

> To be really portable we would use numeric_limits<unsigned char>::max()

> but we don't need to worry about non-8-bit char in our implementation.


I can change that as well so that we don't need to re-think it every time.
Ville Voutilainen Oct. 17, 2018, 12:06 p.m. | #3
On Wed, 17 Oct 2018 at 12:20, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
>

> On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote:

> > It might be worth also calling f<256>() and doing:

> >

> > template <std::size_t... Is>

> > void f_impl(std::index_sequence<Is...> is)

> > {

> >   using V = std::variant<S<Is>...>;

> >

> >   // For a variant of 255 alternatives the valid indices are [0,254]

> >   // and index 255 means valueless-by-exception, so fits in one byte.

> >   if constexpr (std::variant_size_v<V> <= 255)

> >     static_assert(sizeof(V) == 2)

> >   else

> >     static_assert(sizeof(V) > 2)

> > }

> >

> > Just to check we don't introduce an off-by-one error in the *other*

> > direction in future. What do you think?

>

> Agreed. I'll patch that in.

>

> > To be really portable we would use numeric_limits<unsigned char>::max()

> > but we don't need to worry about non-8-bit char in our implementation.

>

> I can change that as well so that we don't need to re-think it every time.


Here.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 2d86a70..55b6440 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -362,7 +362,7 @@ namespace __variant
 
   template <typename... _Types>
   using __select_index =
-    typename __select_int::_Select_int_base<sizeof...(_Types) + 1,
+    typename __select_int::_Select_int_base<sizeof...(_Types),
 					    unsigned char,
 					    unsigned short>::type::value_type;
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/87619.cc b/libstdc++-v3/testsuite/20_util/variant/87619.cc
new file mode 100644
index 0000000..e6f48fe
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/87619.cc
@@ -0,0 +1,53 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target c++17 } }
+
+#include <variant>
+#include <utility>
+#include <limits>
+
+template<std::size_t I>
+struct S {
+};
+
+template <std::size_t... Is>
+void f_impl(std::index_sequence<Is...>)
+{
+	using V = std::variant<S<Is>...>;
+	// For a variant of 255 alternatives the valid indices are [0,254]
+	// and index 255 means valueless-by-exception, so fits in one byte.
+	if constexpr (std::variant_size_v<V> <=
+		      std::numeric_limits<unsigned char>::max())
+	  static_assert(sizeof(V) == 2);
+	else
+	  static_assert(sizeof(V) > 2);
+}
+
+template <std::size_t I>
+void f()
+{
+	f_impl(std::make_index_sequence<I>{});
+}
+
+int main()
+{
+	f<std::numeric_limits<unsigned char>::max() - 1>();
+	f<std::numeric_limits<unsigned char>::max()>();
+	f<std::numeric_limits<unsigned char>::max() + 1>();
+}
Jonathan Wakely Oct. 17, 2018, 12:41 p.m. | #4
On 17/10/18 15:06 +0300, Ville Voutilainen wrote:
>On Wed, 17 Oct 2018 at 12:20, Ville Voutilainen

><ville.voutilainen@gmail.com> wrote:

>>

>> On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote:

>> > It might be worth also calling f<256>() and doing:

>> >

>> > template <std::size_t... Is>

>> > void f_impl(std::index_sequence<Is...> is)

>> > {

>> >   using V = std::variant<S<Is>...>;

>> >

>> >   // For a variant of 255 alternatives the valid indices are [0,254]

>> >   // and index 255 means valueless-by-exception, so fits in one byte.

>> >   if constexpr (std::variant_size_v<V> <= 255)

>> >     static_assert(sizeof(V) == 2)

>> >   else

>> >     static_assert(sizeof(V) > 2)

>> > }

>> >

>> > Just to check we don't introduce an off-by-one error in the *other*

>> > direction in future. What do you think?

>>

>> Agreed. I'll patch that in.

>>

>> > To be really portable we would use numeric_limits<unsigned char>::max()

>> > but we don't need to worry about non-8-bit char in our implementation.

>>

>> I can change that as well so that we don't need to re-think it every time.

>

>Here.


Great, thanks for the changes - OK for trunk.

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 2d86a70..55b6440 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -362,7 +362,7 @@  namespace __variant
 
   template <typename... _Types>
   using __select_index =
-    typename __select_int::_Select_int_base<sizeof...(_Types) + 1,
+    typename __select_int::_Select_int_base<sizeof...(_Types),
 					    unsigned char,
 					    unsigned short>::type::value_type;
 
diff --git a/libstdc++-v3/testsuite/20_util/variant/87619.cc b/libstdc++-v3/testsuite/20_util/variant/87619.cc
new file mode 100644
index 0000000..a8db6b8
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/87619.cc
@@ -0,0 +1,46 @@ 
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target c++17 } }
+
+#include <variant>
+#include <utility>
+
+
+template<std::size_t I>
+struct S {
+};
+
+template <std::size_t... Is>
+void f_impl(std::index_sequence<Is...>)
+{
+	using V = std::variant<S<Is>...>;
+	static_assert(sizeof(V) == 2);
+}
+
+template <std::size_t I>
+void f()
+{
+	f_impl(std::make_index_sequence<I>{});
+}
+
+int main()
+{
+	f<254>();
+	f<255>();
+}