Avoid _Rb_tree_rotate_[left,right] symbols export

Avoid _Rb_tree_rotate_[left,right] symbols export

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

Hi

     When versioned namespace is active we can avoid export of 
_Rb_tree_rotate_[left,right] symbols. I also took the opportunity to put 
static functions in the anonymous namespace rather than using static. Is 
this usage of static still planned to be deprecated ?

     * src/c++98/tree.cc [_GLIBCXX_INLINE_VERSION]
     (_Rb_tree_rotate_left, _Rb_tree_rotate_right): Remove.
     * src/c++98/tree.cc (local_Rb_tree_increment, local_Rb_tree_decrement):
     Move to anonymous namespace.
     (local_Rb_tree_rotate_left, local_Rb_tree_rotate_right): Likewise.

Tested under Linux x86_64 with versioned namespace.

Ok to commit ?

François



[-- Attachment #2: tree.cc.patch --]
[-- Type: text/x-patch, Size: 5269 bytes --]

diff --git a/libstdc++-v3/src/c++98/tree.cc b/libstdc++-v3/src/c++98/tree.cc
index 50fa7cf..89ea108 100644
--- a/libstdc++-v3/src/c++98/tree.cc
+++ b/libstdc++-v3/src/c++98/tree.cc
@@ -52,12 +52,10 @@
 
 #include <bits/stl_tree.h>
 
-namespace std _GLIBCXX_VISIBILITY(default)
+namespace
 {
-_GLIBCXX_BEGIN_NAMESPACE_VERSION
-
-  static _Rb_tree_node_base*
-  local_Rb_tree_increment(_Rb_tree_node_base* __x) throw ()
+  std::_Rb_tree_node_base*
+  local_Rb_tree_increment(std::_Rb_tree_node_base* __x) throw ()
   {
     if (__x->_M_right != 0)
       {
@@ -67,7 +65,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
     else
       {
-        _Rb_tree_node_base* __y = __x->_M_parent;
+	std::_Rb_tree_node_base* __y = __x->_M_parent;
         while (__x == __y->_M_right)
           {
             __x = __y;
@@ -79,34 +77,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return __x;
   }
 
-  _Rb_tree_node_base*
-  _Rb_tree_increment(_Rb_tree_node_base* __x) throw ()
+  std::_Rb_tree_node_base*
+  local_Rb_tree_decrement(std::_Rb_tree_node_base* __x) throw ()
   {
-    return local_Rb_tree_increment(__x);
-  }
-
-  const _Rb_tree_node_base*
-  _Rb_tree_increment(const _Rb_tree_node_base* __x) throw ()
-  {
-    return local_Rb_tree_increment(const_cast<_Rb_tree_node_base*>(__x));
-  }
-
-  static _Rb_tree_node_base*
-  local_Rb_tree_decrement(_Rb_tree_node_base* __x) throw ()
-  {
-    if (__x->_M_color == _S_red
+    if (__x->_M_color == std::_S_red
         && __x->_M_parent->_M_parent == __x)
       __x = __x->_M_right;
     else if (__x->_M_left != 0)
       {
-        _Rb_tree_node_base* __y = __x->_M_left;
+	std::_Rb_tree_node_base* __y = __x->_M_left;
         while (__y->_M_right != 0)
           __y = __y->_M_right;
         __x = __y;
       }
     else
       {
-        _Rb_tree_node_base* __y = __x->_M_parent;
+	std::_Rb_tree_node_base* __y = __x->_M_parent;
         while (__x == __y->_M_left)
           {
             __x = __y;
@@ -117,23 +103,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return __x;
   }
 
-  _Rb_tree_node_base*
-  _Rb_tree_decrement(_Rb_tree_node_base* __x) throw ()
-  {
-    return local_Rb_tree_decrement(__x);
-  }
-
-  const _Rb_tree_node_base*
-  _Rb_tree_decrement(const _Rb_tree_node_base* __x) throw ()
-  {
-    return local_Rb_tree_decrement(const_cast<_Rb_tree_node_base*>(__x));
-  }
-
-  static void
-  local_Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
-		             _Rb_tree_node_base*& __root)
+  void
+  local_Rb_tree_rotate_left(std::_Rb_tree_node_base* const __x,
+			    std::_Rb_tree_node_base*& __root)
   {
-    _Rb_tree_node_base* const __y = __x->_M_right;
+    std::_Rb_tree_node_base* const __y = __x->_M_right;
 
     __x->_M_right = __y->_M_left;
     if (__y->_M_left !=0)
@@ -150,21 +124,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __x->_M_parent = __y;
   }
 
-  /* Static keyword was missing on _Rb_tree_rotate_left.
-     Export the symbol for backward compatibility until
-     next ABI change.  */
   void
-  _Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
-		       _Rb_tree_node_base*& __root)
+  local_Rb_tree_rotate_right(std::_Rb_tree_node_base* const __x,
+			     std::_Rb_tree_node_base*& __root)
   {
-    local_Rb_tree_rotate_left (__x, __root);
-  }
-
-  static void
-  local_Rb_tree_rotate_right(_Rb_tree_node_base* const __x,
-			     _Rb_tree_node_base*& __root)
-  {
-    _Rb_tree_node_base* const __y = __x->_M_left;
+    std::_Rb_tree_node_base* const __y = __x->_M_left;
 
     __x->_M_left = __y->_M_right;
     if (__y->_M_right != 0)
@@ -180,6 +144,46 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __y->_M_right = __x;
     __x->_M_parent = __y;
   }
+}
+
+namespace std _GLIBCXX_VISIBILITY(default)
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+
+  _Rb_tree_node_base*
+  _Rb_tree_increment(_Rb_tree_node_base* __x) throw ()
+  {
+    return local_Rb_tree_increment(__x);
+  }
+
+  const _Rb_tree_node_base*
+  _Rb_tree_increment(const _Rb_tree_node_base* __x) throw ()
+  {
+    return local_Rb_tree_increment(const_cast<_Rb_tree_node_base*>(__x));
+  }
+
+  _Rb_tree_node_base*
+  _Rb_tree_decrement(_Rb_tree_node_base* __x) throw ()
+  {
+    return local_Rb_tree_decrement(__x);
+  }
+
+  const _Rb_tree_node_base*
+  _Rb_tree_decrement(const _Rb_tree_node_base* __x) throw ()
+  {
+    return local_Rb_tree_decrement(const_cast<_Rb_tree_node_base*>(__x));
+  }
+
+#if !_GLIBCXX_INLINE_VERSION
+  /* Static keyword was missing on _Rb_tree_rotate_left.
+     Export the symbol for backward compatibility until
+     next ABI change.  */
+  void
+  _Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
+		       _Rb_tree_node_base*& __root)
+  {
+    local_Rb_tree_rotate_left (__x, __root);
+  }
 
   /* Static keyword was missing on _Rb_tree_rotate_right
      Export the symbol for backward compatibility until
@@ -190,9 +194,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
     local_Rb_tree_rotate_right (__x, __root);
   }
+#endif
 
   void
-  _Rb_tree_insert_and_rebalance(const bool          __insert_left,
+  _Rb_tree_insert_and_rebalance(const bool __insert_left,
                                 _Rb_tree_node_base* __x,
                                 _Rb_tree_node_base* __p,
                                 _Rb_tree_node_base& __header) throw ()

Re: Avoid _Rb_tree_rotate_[left,right] symbols export

[Jonathan Wakely]

On 11/05/17 22:06 +0200, François Dumont wrote:
>Hi
>
>    When versioned namespace is active we can avoid export of 
>_Rb_tree_rotate_[left,right] symbols. I also took the opportunity to 
>put static functions in the anonymous namespace rather than using 
>static. Is this usage of static still planned to be deprecated ?

No, I don't think so.

A much simpler (but equivalent) change would be:

--- a/libstdc++-v3/src/c++98/tree.cc
+++ b/libstdc++-v3/src/c++98/tree.cc
@@ -153,6 +153,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   /* Static keyword was missing on _Rb_tree_rotate_left.
      Export the symbol for backward compatibility until
      next ABI change.  */
+#if _GLIBCXX_INLINE_VERSION
+  static
+#endif
   void
   _Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
                       _Rb_tree_node_base*& __root)
@@ -184,6 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   /* Static keyword was missing on _Rb_tree_rotate_right
      Export the symbol for backward compatibility until
      next ABI change.  */
+#if _GLIBCXX_INLINE_VERSION
+  static
+#endif
   void
   _Rb_tree_rotate_right(_Rb_tree_node_base* const __x,
                        _Rb_tree_node_base*& __root)



>    * src/c++98/tree.cc [_GLIBCXX_INLINE_VERSION]
>    (_Rb_tree_rotate_left, _Rb_tree_rotate_right): Remove.
>    * src/c++98/tree.cc (local_Rb_tree_increment, local_Rb_tree_decrement):
>    Move to anonymous namespace.
>    (local_Rb_tree_rotate_left, local_Rb_tree_rotate_right): Likewise.
>
>Tested under Linux x86_64 with versioned namespace.

What about the normal configuration? It's much more important that the
default configuration works. The versioned namespace that nobody uses
doesn't matter.

Re: Avoid _Rb_tree_rotate_[left,right] symbols export

[François Dumont]

On 12/05/2017 13:03, Jonathan Wakely wrote:
> On 11/05/17 22:06 +0200, François Dumont wrote:
>> Hi
>>
>>    When versioned namespace is active we can avoid export of 
>> _Rb_tree_rotate_[left,right] symbols. I also took the opportunity to 
>> put static functions in the anonymous namespace rather than using 
>> static. Is this usage of static still planned to be deprecated ?
>
> No, I don't think so.

So not interested in replacing it with anonymous namespace ?

>
> A much simpler (but equivalent) change would be:
>
> --- a/libstdc++-v3/src/c++98/tree.cc
> +++ b/libstdc++-v3/src/c++98/tree.cc
> @@ -153,6 +153,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   /* Static keyword was missing on _Rb_tree_rotate_left.
>      Export the symbol for backward compatibility until
>      next ABI change.  */
> +#if _GLIBCXX_INLINE_VERSION
> +  static
> +#endif

Surely simpler but why keeping this function if it is not used ?

> void
>   _Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
>                       _Rb_tree_node_base*& __root)
> @@ -184,6 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   /* Static keyword was missing on _Rb_tree_rotate_right
>      Export the symbol for backward compatibility until
>      next ABI change.  */
> +#if _GLIBCXX_INLINE_VERSION
> +  static
> +#endif
>   void
>   _Rb_tree_rotate_right(_Rb_tree_node_base* const __x,
>                        _Rb_tree_node_base*& __root)
>
>
>
>>    * src/c++98/tree.cc [_GLIBCXX_INLINE_VERSION]
>>    (_Rb_tree_rotate_left, _Rb_tree_rotate_right): Remove.
>>    * src/c++98/tree.cc (local_Rb_tree_increment, 
>> local_Rb_tree_decrement):
>>    Move to anonymous namespace.
>>    (local_Rb_tree_rotate_left, local_Rb_tree_rotate_right): Likewise.
>>
>> Tested under Linux x86_64 with versioned namespace.
>
> What about the normal configuration? It's much more important that the
> default configuration works. The versioned namespace that nobody uses
> doesn't matter.
>
>
Normal mode now tested too, success.

François

Re: Avoid _Rb_tree_rotate_[left,right] symbols export

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On 12/05/2017 13:03, Jonathan Wakely wrote:
> A much simpler (but equivalent) change would be:
>
> --- a/libstdc++-v3/src/c++98/tree.cc
> +++ b/libstdc++-v3/src/c++98/tree.cc
> @@ -153,6 +153,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   /* Static keyword was missing on _Rb_tree_rotate_left.
>      Export the symbol for backward compatibility until
>      next ABI change.  */
> +#if _GLIBCXX_INLINE_VERSION
> +  static
> +#endif

Ok, so it looks like you are not a great fan of the anonymous namespace 
in this context.

Here is a new proposal. We don't need to add static keyword, this 
function is only here to be exported for backward compatibility.

>> Tested under Linux x86_64 with versioned namespace.
>
> What about the normal configuration? It's much more important that the
> default configuration works. The versioned namespace that nobody uses
> doesn't matter.

Tested under Linux x86_64 normal mode.

Ok to commit ?

François



[-- Attachment #2: tree.cc.patch --]
[-- Type: text/x-patch, Size: 1284 bytes --]

diff --git a/libstdc++-v3/src/c++98/tree.cc b/libstdc++-v3/src/c++98/tree.cc
index 50fa7cf..0984b05 100644
--- a/libstdc++-v3/src/c++98/tree.cc
+++ b/libstdc++-v3/src/c++98/tree.cc
@@ -150,15 +150,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __x->_M_parent = __y;
   }
 
+#if !_GLIBCXX_INLINE_VERSION
   /* Static keyword was missing on _Rb_tree_rotate_left.
      Export the symbol for backward compatibility until
      next ABI change.  */
   void
   _Rb_tree_rotate_left(_Rb_tree_node_base* const __x,
 		       _Rb_tree_node_base*& __root)
-  {
-    local_Rb_tree_rotate_left (__x, __root);
-  }
+  { local_Rb_tree_rotate_left (__x, __root); }
+#endif
 
   static void
   local_Rb_tree_rotate_right(_Rb_tree_node_base* const __x,
@@ -181,15 +181,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __x->_M_parent = __y;
   }
 
+#if !_GLIBCXX_INLINE_VERSION
   /* Static keyword was missing on _Rb_tree_rotate_right
      Export the symbol for backward compatibility until
      next ABI change.  */
   void
   _Rb_tree_rotate_right(_Rb_tree_node_base* const __x,
 			_Rb_tree_node_base*& __root)
-  {
-    local_Rb_tree_rotate_right (__x, __root);
-  }
+  { local_Rb_tree_rotate_right (__x, __root); }
+#endif
 
   void
   _Rb_tree_insert_and_rebalance(const bool          __insert_left,

Re: Avoid _Rb_tree_rotate_[left,right] symbols export

[Jonathan Wakely]

On 13/06/17 21:51 +0200, François Dumont wrote:
>On 12/05/2017 13:03, Jonathan Wakely wrote:
>>A much simpler (but equivalent) change would be:
>>
>>--- a/libstdc++-v3/src/c++98/tree.cc
>>+++ b/libstdc++-v3/src/c++98/tree.cc
>>@@ -153,6 +153,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>  /* Static keyword was missing on _Rb_tree_rotate_left.
>>     Export the symbol for backward compatibility until
>>     next ABI change.  */
>>+#if _GLIBCXX_INLINE_VERSION
>>+  static
>>+#endif
>
>Ok, so it looks like you are not a great fan of the anonymous 
>namespace in this context.
>
>Here is a new proposal. We don't need to add static keyword, this 
>function is only here to be exported for backward compatibility.
>
>>>Tested under Linux x86_64 with versioned namespace.
>>
>>What about the normal configuration? It's much more important that the
>>default configuration works. The versioned namespace that nobody uses
>>doesn't matter.
>
>Tested under Linux x86_64 normal mode.
>
>Ok to commit ?

OK, thanks.