Re: [v3 PATCH] Reduce the size of variant, it doesn't need an index of type size_t internally.

2017-01-11 Thread Jonathan Wakely 

On 11/01/17 10:29 +, Jonathan Wakely wrote:

On 11/01/17 00:19 +0200, Ville Voutilainen wrote:

@@ -1086,7 +1099,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 { return !this->_M_valid(); }

 constexpr size_t index() const noexcept
-  { return this->_M_index; }
+  {
+   if (this->_M_index ==
+   typename _Base::_Storage::__index_type(variant_npos))
+ return variant_npos;
+   return this->_M_index;


GCC doesn't seem to be smart enough to optimize the branch away here.


But that's only for 32-bit x86. It optimizes well for x86_64, so no
need to obfuscate it.

Re: [v3 PATCH] Reduce the size of variant, it doesn't need an index of type size_t internally.

2017-01-11 Thread Jonathan Wakely 

On 11/01/17 00:19 +0200, Ville Voutilainen wrote:

@@ -1086,7 +1099,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  { return !this->_M_valid(); }

  constexpr size_t index() const noexcept
-  { return this->_M_index; }
+  {
+   if (this->_M_index ==
+   typename _Base::_Storage::__index_type(variant_npos))
+ return variant_npos;
+   return this->_M_index;


GCC doesn't seem to be smart enough to optimize the branch away here.
Something like this would avoid it:

 constexpr size_t index const noexcept
 {
   using __index_type = typename _Base::_Storage::__index_type;
   return size_t(__index_type(this->_M_index + 1)) - 1;
 }

But we can worry about that later.


+  }

  void
  swap(variant& __rhs)
diff --git a/libstdc++-v3/testsuite/20_util/variant/index_type.cc 
b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
new file mode 100644
index 000..b7f3a7b
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
@@ -0,0 +1,24 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target x86_64-*-* powerpc*-*-*} }


Please add a space after the second target (it seems to work anyway,
but still better to add it).

OK for trunk with that space character added, thanks.

Re: [v3 PATCH] Reduce the size of variant, it doesn't need an index of type size_t internally.

2017-01-10 Thread Tim Shen via gcc-patches 
On Tue, Jan 10, 2017 at 2:19 PM, Ville Voutilainen
 wrote:
> Cleanups based on review; there's no longer any public typedefs added
> to variant,
> and the test is greatly simpler with much less trickery.

Looks good to me.

Thanks!


-- 
Regards,
Tim Shen

Re: [v3 PATCH] Reduce the size of variant, it doesn't need an index of type size_t internally.

2017-01-10 Thread Ville Voutilainen 
Cleanups based on review; there's no longer any public typedefs added
to variant,
and the test is greatly simpler with much less trickery.

2017-01-11  Ville Voutilainen  

Reduce the size of variant, it doesn't need an index of
type size_t internally.
* include/std/variant (parse_numbers.h): New include.
(__select_index): New.
(_Variant_storage::_M_reset_impl): Use
_index_type for comparison with variant_npos.
(_Variant_storage::__index_type): New.
(_Variant_storage::_M_index): Change the
type from size_t to __index_type.
(_Variant_storage::__index_type): New.
(_Variant_storage::_M_index): Change the
type from size_t to __index_type.
(_Variant_base::_M_valid): Use _Storage::__index_type
for comparison with variant_npos.
(variant::index): Use _Base::_Storage::__index_type
for comparison with variant_npos.
* testsuite/20_util/variant/index_type.cc: New.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 3d025a7..6404fce 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
@@ -314,6 +315,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
 struct _Variant_storage;
 
+  template 
+  using __select_index =
+typename __select_int::_Select_int_base
+::type::value_type;
+
   template
 struct _Variant_storage
 {
@@ -332,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
constexpr void _M_reset_impl(std::index_sequence<__indices...>)
{
- if (_M_index != variant_npos)
+ if (_M_index != __index_type(variant_npos))
_S_vtable<__indices...>[_M_index](*this);
}
 
@@ -346,7 +354,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { _M_reset(); }
 
   _Variadic_union<_Types...> _M_u;
-  size_t _M_index;
+  using __index_type = __select_index<_Types...>;
+  __index_type _M_index;
 };
 
   template
@@ -364,7 +373,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { _M_index = variant_npos; }
 
   _Variadic_union<_Types...> _M_u;
-  size_t _M_index;
+  using __index_type = __select_index<_Types...>;
+  __index_type _M_index;
 };
 
   // Helps SFINAE on special member functions. Otherwise it can live in variant
@@ -487,7 +497,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   constexpr bool
   _M_valid() const noexcept
-  { return this->_M_index != variant_npos; }
+  {
+   return this->_M_index !=
+ typename _Storage::__index_type(variant_npos);
+  }
 };
 
   // For how many times does _Tp appear in _Tuple?
@@ -1086,7 +1099,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { return !this->_M_valid(); }
 
   constexpr size_t index() const noexcept
-  { return this->_M_index; }
+  {
+   if (this->_M_index ==
+   typename _Base::_Storage::__index_type(variant_npos))
+ return variant_npos;
+   return this->_M_index;
+  }
 
   void
   swap(variant& __rhs)
diff --git a/libstdc++-v3/testsuite/20_util/variant/index_type.cc 
b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
new file mode 100644
index 000..b7f3a7b
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
@@ -0,0 +1,24 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do compile { target x86_64-*-* powerpc*-*-*} }
+
+// Copyright (C) 2017 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
+// .
+
+#include 
+
+static_assert(sizeof(std::variant)
+ < sizeof(size_t));

[v3 PATCH] Reduce the size of variant, it doesn't need an index of type size_t internally.

2017-01-09 Thread Ville Voutilainen 
Tested on Linux-x64.

2017-01-09  Ville Voutilainen  

Reduce the size of variant, it doesn't need an index of
type size_t internally.
* include/std/variant (parse_numbers.h): New include.
(__select_index): New.
(_Variant_storage::_M_reset_impl): Use
_index_type for comparison with variant_npos.
(_Variant_storage::__index_type): New.
(_Variant_storage::_M_index): Change the
type from size_t to __index_type.
(_Variant_storage::__index_type): New.
(_Variant_storage::_M_index): Change the
type from size_t to __index_type.
(_Variant_base::_M_valid): Use __index_type for comparison
with variant_npos.
(variant::__index_type): New.
(variant::index): Use __index_type for comparison with variant_npos.
* testsuite/20_util/variant/index_type.cc: New.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 3d025a7..b016f32 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
@@ -314,6 +315,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
 struct _Variant_storage;
 
+  template 
+  using __select_index =
+typename __select_int::_Select_int_base
+::type::value_type;
+
   template
 struct _Variant_storage
 {
@@ -332,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template
constexpr void _M_reset_impl(std::index_sequence<__indices...>)
{
- if (_M_index != variant_npos)
+ if (_M_index != __index_type(variant_npos))
_S_vtable<__indices...>[_M_index](*this);
}
 
@@ -346,7 +354,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { _M_reset(); }
 
   _Variadic_union<_Types...> _M_u;
-  size_t _M_index;
+  using __index_type = __select_index<_Types...>;
+  __index_type _M_index;
 };
 
   template
@@ -364,7 +373,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { _M_index = variant_npos; }
 
   _Variadic_union<_Types...> _M_u;
-  size_t _M_index;
+  using __index_type = __select_index<_Types...>;
+  __index_type _M_index;
 };
 
   // Helps SFINAE on special member functions. Otherwise it can live in variant
@@ -487,7 +497,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   constexpr bool
   _M_valid() const noexcept
-  { return this->_M_index != variant_npos; }
+  {
+   return this->_M_index !=
+ typename _Storage::__index_type(variant_npos);
+  }
 };
 
   // For how many times does _Tp appear in _Tuple?
@@ -944,6 +957,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  __detail::__variant::__index_of_v<_Tp, _Types...>;
 
 public:
+  using __index_type = typename _Base::_Storage::__index_type;
   constexpr variant()
   noexcept(is_nothrow_default_constructible_v<__to_type<0>>) = default;
   variant(const variant&) = default;
@@ -1086,7 +1100,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { return !this->_M_valid(); }
 
   constexpr size_t index() const noexcept
-  { return this->_M_index; }
+  {
+   if (this->_M_index == __index_type(variant_npos))
+ return variant_npos;
+   return this->_M_index;
+  }
 
   void
   swap(variant& __rhs)
diff --git a/libstdc++-v3/testsuite/20_util/variant/index_type.cc 
b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
new file mode 100644
index 000..ea6b4d6
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/index_type.cc
@@ -0,0 +1,43 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do compile }
+
+// Copyright (C) 2017 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
+// .
+
+#include 
+#include 
+#include 
+
+template  struct Bogus {};
+
+template  auto f(std::index_sequence)
+{
+  return std::variant...>{};
+}
+
+static_assert(sizeof(char) >= sizeof(int) ||
+ CHAR_BIT != 8 ||
+ std::is_same_v<
+ decltype(f(std::make_index_sequence<3>()))::__index_type,
+ unsigned char>);
+
+static_assert(sizeof(char) >= sizeof(int) ||
+ sizeof(char) >= sizeof(short) ||
+ CHAR_BIT != 8 ||
+ std::is_same_v<
+ decltype(f(std::make_index_sequence<260>()))::__index_type,
+ unsigned short>);