diff mbox series

Share ebo helper throughout lib

Message ID 1b6ec4c9-cdf9-2b7c-f263-185e70029d54@gmail.com
State New
Headers show
Series Share ebo helper throughout lib | expand

Commit Message

François Dumont July 25, 2018, 7:42 p.m. UTC
Hi

     It has already been noticed that there are 2 ebo helpers in the 
lib. Here is a patch to use 1.


     * include/bits/ebo_helper.h: New.
     * include/Makefile.am: Add latter.
     * include/Makefile.in: Regenerate.
     * include/bits/hashtable_policy.h: Adapt.
     * include/bits/shared_ptr_base.h: Adapt.

Tested under linux x86_64.

Ok to commit ?

François

Comments

Marc Glisse July 25, 2018, 7:53 p.m. UTC | #1
On Wed, 25 Jul 2018, François Dumont wrote:

>     It has already been noticed that there are 2 ebo helpers in the lib. Here 
> is a patch to use 1.
>
>
>     * include/bits/ebo_helper.h: New.
>     * include/Makefile.am: Add latter.
>     * include/Makefile.in: Regenerate.
>     * include/bits/hashtable_policy.h: Adapt.
>     * include/bits/shared_ptr_base.h: Adapt.
>
> Tested under linux x86_64.
>
> Ok to commit ?

I don't think we support [[no_unique_address]] yet, but assuming we soon 
will and we enable it also for C++03 (at least with the __attribute__ 
syntax and/or in system headers), do you know if some similar helper will 
still be necessary, with a simpler implementation, or if the attribute 
will magically get rid of it?

(I haven't looked at it at all, the answer may be obvious)
Jonathan Wakely July 25, 2018, 7:58 p.m. UTC | #2
On 25/07/18 21:42 +0200, François Dumont wrote:
>Hi
>
>    It has already been noticed that there are 2 ebo helpers in the 
>lib. Here is a patch to use 1.
>
>
>    * include/bits/ebo_helper.h: New.
>    * include/Makefile.am: Add latter.
>    * include/Makefile.in: Regenerate.
>    * include/bits/hashtable_policy.h: Adapt.
>    * include/bits/shared_ptr_base.h: Adapt.

I think we want an extra template parameter which is used for a tag
type, to guarantee that the two uses (in hash tables and in shared
ptr) can never conflict and produce ambiguous bases.

i.e.

  template<int _Nm, typename _Tp, typename _Tag,
         bool __use_ebo = !__is_final(_Tp) && __is_empty(_Tp)>
    struct _Ebo_helper;

and:

using _Sp_ebo_helper = __detail::_Ebo_helper<_Nm, _Tp, _Sp_counted_base<>>;

(for example).
Jonathan Wakely July 25, 2018, 8:04 p.m. UTC | #3
On 25/07/18 21:53 +0200, Marc Glisse wrote:
>On Wed, 25 Jul 2018, François Dumont wrote:
>
>>    It has already been noticed that there are 2 ebo helpers in the 
>>lib. Here is a patch to use 1.
>>
>>
>>    * include/bits/ebo_helper.h: New.
>>    * include/Makefile.am: Add latter.
>>    * include/Makefile.in: Regenerate.
>>    * include/bits/hashtable_policy.h: Adapt.
>>    * include/bits/shared_ptr_base.h: Adapt.
>>
>>Tested under linux x86_64.
>>
>>Ok to commit ?
>
>I don't think we support [[no_unique_address]] yet, but assuming we 
>soon will and we enable it also for C++03 (at least with the 
>__attribute__ syntax and/or in system headers), do you know if some 

Yes, I hope we'll have that soon.

>similar helper will still be necessary, with a simpler implementation, 
>or if the attribute will magically get rid of it?

We'll be able to replace some uses of EBO with the attribute
(specifically, in std::tuple). In some places we'll want to only apply
the attribute under the same conditions as we currently use the EBO,
because otherwise we'd change the layout ("compressing" the member
using the attribute where we previously didn't compress it).

In some cases that will be OK because it's an internal implementation
detail, or because we can replace e.g. _Sp_counted_deleter with
_Sp_counted_deleter_v2. In other cases we must avoid any layout change
(e.g. std::tuple).

Concretely, we probably don't want to change the layout of the
hashtable types. We could change the layout for shared_ptr
_Sp_counted_xxx types (gaining some additional space savings for final
types that currently can't be EBO'd) as long as we rename them to
avoid the linker trying to combine incompatible definitions. So on
that basis, maybe we don't want to bother changing the _Sp_ebo_helper
for now.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
index 70db3cb..98c1a6c 100644
--- a/libstdc++-v3/include/Makefile.am
+++ b/libstdc++-v3/include/Makefile.am
@@ -105,6 +105,7 @@  bits_headers = \
 	${bits_srcdir}/concept_check.h \
 	${bits_srcdir}/cpp_type_traits.h \
 	${bits_srcdir}/deque.tcc \
+	${bits_srcdir}/ebo_helper.h \
 	${bits_srcdir}/enable_special_members.h \
 	${bits_srcdir}/forward_list.h \
 	${bits_srcdir}/forward_list.tcc \
diff --git a/libstdc++-v3/include/Makefile.in b/libstdc++-v3/include/Makefile.in
index 0e1cbe4..35093bc 100644
--- a/libstdc++-v3/include/Makefile.in
+++ b/libstdc++-v3/include/Makefile.in
@@ -398,6 +398,7 @@  bits_headers = \
 	${bits_srcdir}/concept_check.h \
 	${bits_srcdir}/cpp_type_traits.h \
 	${bits_srcdir}/deque.tcc \
+	${bits_srcdir}/ebo_helper.h \
 	${bits_srcdir}/enable_special_members.h \
 	${bits_srcdir}/forward_list.h \
 	${bits_srcdir}/forward_list.tcc \
diff --git a/libstdc++-v3/include/bits/ebo_helper.h b/libstdc++-v3/include/bits/ebo_helper.h
new file mode 100644
index 0000000..5b9073a
--- /dev/null
+++ b/libstdc++-v3/include/bits/ebo_helper.h
@@ -0,0 +1,114 @@ 
+// Ebo helper header -*- C++ -*-
+
+// 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.
+
+// Under Section 7 of GPL version 3, you are granted additional
+// permissions described in the GCC Runtime Library Exception, version
+// 3.1, as published by the Free Software Foundation.
+
+// You should have received a copy of the GNU General Public License and
+// a copy of the GCC Runtime Library Exception along with this program;
+// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+// <http://www.gnu.org/licenses/>.
+
+/** @file bits/ebo_helper.h
+ *  This is an internal header file, included by other library headers.
+ *  Do not attempt to use it directly.
+ *  @headername{unordered_map,unordered_set,memory}
+ */
+
+#ifndef _EBO_HELPER_H
+#define _EBO_HELPER_H 1
+
+#include <bits/move.h>
+
+namespace std _GLIBCXX_VISIBILITY(default)
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+
+namespace __detail
+{
+  /**
+   *  Primary class template _Ebo_helper.
+   *
+   *  Helper class using EBO when it is not forbidden (the type is not
+   *  final) and when it is worth it (the type is empty.)
+   */
+  template<int _Nm, typename _Tp,
+	   bool __use_ebo = !__is_final(_Tp) && __is_empty(_Tp)>
+    struct _Ebo_helper;
+
+  /// Specialization using EBO.
+  template<int _Nm, typename _Tp>
+    struct _Ebo_helper<_Nm, _Tp, true>
+    : private _Tp
+    {
+      _Ebo_helper() = default;
+      _Ebo_helper(const _Tp& __tp)
+      : _Tp(__tp)
+      { }
+
+      _Ebo_helper(_Tp&& __tp)
+      : _Tp(std::move(__tp))
+      { }
+
+      template<typename _OtherTp>
+	_Ebo_helper(_OtherTp&& __tp)
+	: _Tp(std::forward<_OtherTp>(__tp))
+	{ }
+
+      static const _Tp&
+      _S_cget(const _Ebo_helper& __eboh)
+      { return __eboh; }
+
+      static _Tp&
+      _S_get(_Ebo_helper& __eboh)
+      { return __eboh; }
+    };
+
+  /// Specialization not using EBO.
+  template<int _Nm, typename _Tp>
+    struct _Ebo_helper<_Nm, _Tp, false>
+    {
+      _Ebo_helper() = default;
+      _Ebo_helper(const _Tp& __tp)
+      : _M_tp(__tp)
+      { }
+
+      _Ebo_helper(_Tp&& __tp)
+      : _M_tp(std::move(__tp))
+      { }
+
+      template<typename _OtherTp>
+	_Ebo_helper(_OtherTp&& __tp)
+	: _M_tp(std::forward<_OtherTp>(__tp))
+	{ }
+
+      static const _Tp&
+      _S_cget(const _Ebo_helper& __eboh)
+      { return __eboh._M_tp; }
+
+      static _Tp&
+      _S_get(_Ebo_helper& __eboh)
+      { return __eboh._M_tp; }
+
+    private:
+      _Tp _M_tp;
+    };
+}
+
+_GLIBCXX_END_NAMESPACE_VERSION
+}
+
+#endif
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index 3ff6b14..62f6fd1 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -34,6 +34,7 @@ 
 #include <tuple>		// for std::tuple, std::forward_as_tuple
 #include <cstdint>		// for std::uint_fast64_t
 #include <bits/stl_algobase.h>	// for std::min.
+#include <bits/ebo_helper.h>
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
@@ -1088,59 +1089,8 @@  namespace __detail
       }
     };
 
-  /**
-   *  Primary class template _Hashtable_ebo_helper.
-   *
-   *  Helper class using EBO when it is not forbidden (the type is not
-   *  final) and when it is worth it (the type is empty.)
-   */
-  template<int _Nm, typename _Tp,
-	   bool __use_ebo = !__is_final(_Tp) && __is_empty(_Tp)>
-    struct _Hashtable_ebo_helper;
-
-  /// Specialization using EBO.
-  template<int _Nm, typename _Tp>
-    struct _Hashtable_ebo_helper<_Nm, _Tp, true>
-    : private _Tp
-    {
-      _Hashtable_ebo_helper() = default;
-
-      template<typename _OtherTp>
-	_Hashtable_ebo_helper(_OtherTp&& __tp)
-	  : _Tp(std::forward<_OtherTp>(__tp))
-	{ }
-
-      static const _Tp&
-      _S_cget(const _Hashtable_ebo_helper& __eboh)
-      { return static_cast<const _Tp&>(__eboh); }
-
-      static _Tp&
-      _S_get(_Hashtable_ebo_helper& __eboh)
-      { return static_cast<_Tp&>(__eboh); }
-    };
-
-  /// Specialization not using EBO.
   template<int _Nm, typename _Tp>
-    struct _Hashtable_ebo_helper<_Nm, _Tp, false>
-    {
-      _Hashtable_ebo_helper() = default;
-
-      template<typename _OtherTp>
-	_Hashtable_ebo_helper(_OtherTp&& __tp)
-	  : _M_tp(std::forward<_OtherTp>(__tp))
-	{ }
-
-      static const _Tp&
-      _S_cget(const _Hashtable_ebo_helper& __eboh)
-      { return __eboh._M_tp; }
-
-      static _Tp&
-      _S_get(_Hashtable_ebo_helper& __eboh)
-      { return __eboh._M_tp; }
-
-    private:
-      _Tp _M_tp;
-    };
+    using _Hashtable_ebo_helper = _Ebo_helper<_Nm, _Tp>;
 
   /**
    *  Primary class template _Local_iterator_base.
diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h b/libstdc++-v3/include/bits/shared_ptr_base.h
index 870aeb9..191e4be 100644
--- a/libstdc++-v3/include/bits/shared_ptr_base.h
+++ b/libstdc++-v3/include/bits/shared_ptr_base.h
@@ -53,6 +53,7 @@ 
 #include <bits/allocated_ptr.h>
 #include <bits/refwrap.h>
 #include <bits/stl_function.h>
+#include <bits/ebo_helper.h>
 #include <ext/aligned_buffer.h>
 
 namespace std _GLIBCXX_VISIBILITY(default)
@@ -403,35 +404,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline void
     _Sp_counted_ptr<nullptr_t, _S_atomic>::_M_dispose() noexcept { }
 
-  template<int _Nm, typename _Tp,
-	   bool __use_ebo = !__is_final(_Tp) && __is_empty(_Tp)>
-    struct _Sp_ebo_helper;
-
-  /// Specialization using EBO.
-  template<int _Nm, typename _Tp>
-    struct _Sp_ebo_helper<_Nm, _Tp, true> : private _Tp
-    {
-      explicit _Sp_ebo_helper(const _Tp& __tp) : _Tp(__tp) { }
-      explicit _Sp_ebo_helper(_Tp&& __tp) : _Tp(std::move(__tp)) { }
-
-      static _Tp&
-      _S_get(_Sp_ebo_helper& __eboh) { return static_cast<_Tp&>(__eboh); }
-    };
-
-  /// Specialization not using EBO.
   template<int _Nm, typename _Tp>
-    struct _Sp_ebo_helper<_Nm, _Tp, false>
-    {
-      explicit _Sp_ebo_helper(const _Tp& __tp) : _M_tp(__tp) { }
-      explicit _Sp_ebo_helper(_Tp&& __tp) : _M_tp(std::move(__tp)) { }
-
-      static _Tp&
-      _S_get(_Sp_ebo_helper& __eboh)
-      { return __eboh._M_tp; }
-
-    private:
-      _Tp _M_tp;
-    };
+    using _Sp_ebo_helper = __detail::_Ebo_helper<_Nm, _Tp>;
 
   // Support for custom deleter and/or allocator
   template<typename _Ptr, typename _Deleter, typename _Alloc, _Lock_policy _Lp>