Re: [v3 PATCH] PR libstdc++/87619

[Jonathan Wakely]

On 17/10/18 15:06 +0300, Ville Voutilainen wrote:

On Wed, 17 Oct 2018 at 12:20, Ville Voutilainen
 wrote:


On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely  wrote:
> It might be worth also calling f<256>() and doing:
>
> template 
> void f_impl(std::index_sequence is)
> {
>   using V = std::variant...>;
>
>   // 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 <= 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::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.

Re: [v3 PATCH] PR libstdc++/87619

[Ville Voutilainen]

On Wed, 17 Oct 2018 at 12:20, Ville Voutilainen
 wrote:
>
> On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely  wrote:
> > It might be worth also calling f<256>() and doing:
> >
> > template 
> > void f_impl(std::index_sequence is)
> > {
> >   using V = std::variant...>;
> >
> >   // 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 <= 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::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 
   using __select_index =
-typename __select_int::_Select_int_base::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 000..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
+// .
+
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target c++17 } }
+
+#include 
+#include 
+#include 
+
+template
+struct S {
+};
+
+template 
+void f_impl(std::index_sequence)
+{
+	using V = std::variant...>;
+	// 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 <=
+		  std::numeric_limits::max())
+	  static_assert(sizeof(V) == 2);
+	else
+	  static_assert(sizeof(V) > 2);
+}
+
+template 
+void f()
+{
+	f_impl(std::make_index_sequence{});
+}
+
+int main()
+{
+	f::max() - 1>();
+	f::max()>();
+	f::max() + 1>();
+}

Re: [v3 PATCH] PR libstdc++/87619

[Ville Voutilainen]

On Wed, 17 Oct 2018 at 12:07, Jonathan Wakely  wrote:
> It might be worth also calling f<256>() and doing:
>
> template 
> void f_impl(std::index_sequence is)
> {
>   using V = std::variant...>;
>
>   // 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 <= 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::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.

Re: [v3 PATCH] PR libstdc++/87619

[Jonathan Wakely]

On 16/10/18 22:59 +0300, Ville Voutilainen wrote:

Simple, short, and sweet, just a thinko.

2018-10-16  Ville Voutilainen  

   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 
  using __select_index =
-typename __select_int::_Select_int_base::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 000..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
+// .
+
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target c++17 } }
+
+#include 
+#include 
+
+
+template
+struct S {
+};
+
+template 
+void f_impl(std::index_sequence)
+{
+   using V = std::variant...>;
+   static_assert(sizeof(V) == 2);


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

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

 // 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 <= 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::max()
but we don't need to worry about non-8-bit char in our implementation.


+}
+
+template 
+void f()
+{
+   f_impl(std::make_index_sequence{});
+}
+
+int main()
+{
+   f<254>();
+   f<255>();
+}