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

[Jonathan Wakely <jwakely at redhat dot com>]

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>();
> +}