| Message ID | CAFk2RUYRRCtoFro2zZzt7YXg4Z02=-+kCzPqTaRywYVDwcu0rQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PR libstdc++/87619 | expand |
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>(); >+}
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.
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>(); +}
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.
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>(); +}
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.