diff mbox series

Replace __gnu_cxx::__ops::__negate with std::not_fn

Message ID 9fbe09f1-ea49-b520-251b-faba47d74179@gmail.com
State New
Headers show
Series Replace __gnu_cxx::__ops::__negate with std::not_fn | expand

Commit Message

François Dumont May 22, 2023, 8:50 p.m. UTC
I was thinking that it might be nice to get rid of predefined_ops.h content.

So here is a start with __negate. Drawback is that stl_algo.h has to 
include <functional>. For now I just get rid of stl_algo.h include in 
<functional> to rather use stl_algobase.h. But maybe it would be better 
to also isolate std::not_fn in a dedicated header file so that 
stl_algo.h do not have to include all <functional>.

     libstdc++: Replace __gnu_cxx::__ops::__negate with std::not_fn

     Replace the internal __gnu_cxx::__ops::__negate function and associated
     __gnu_cxx::__ops::_Iter_negate by the C++17 std::not_fn.

     libstdc++-v3/ChangeLog:

             * include/bits/predefined_ops.h: Include <version>.
             [__cpp_lib_not_fn](__gnu_cxx::__ops::_Iter_negate): Remove.
             [__cpp_lib_not_fn](__gnu_cxx::__ops::__negate): Remove.
             * include/bits/stl_algo.h: Include <functional> for C++17 
and later.
             [__cpp_lib_not_fn](__find_if_not): Use std::not_fn.
             (std::__search, std::search(_FwdIt1, _FwdIt1, _FwdIt2, 
_FwdIt2, _BinPred)): Move...
             * include/bits/stl_algobase.h: ...here.
             * include/std/functional: Replace <stl_algo.h> include by 
<stl_algobase.h>.

Tests still running.

François

Comments

Jonathan Wakely May 22, 2023, 8:55 p.m. UTC | #1
On Mon, 22 May 2023 at 21:51, François Dumont via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> I was thinking that it might be nice to get rid of predefined_ops.h
> content.
>
> So here is a start with __negate. Drawback is that stl_algo.h has to
> include <functional>.


We definitely don't want that. std::not_fn could be move to its own header.

But I'm not sure this is a good change anyway, as we can't do it
unconditionally. Pre-C++17 code would still be using the predefined_ops.h
function objects, so we can't remove that code. And we'll get template
bloat from instantiating the algos twice, once with the old function
objects and once with std::not_fn.



> For now I just get rid of stl_algo.h include in
> <functional> to rather use stl_algobase.h. But maybe it would be better
> to also isolate std::not_fn in a dedicated header file so that
> stl_algo.h do not have to include all <functional>.
>
>      libstdc++: Replace __gnu_cxx::__ops::__negate with std::not_fn
>
>      Replace the internal __gnu_cxx::__ops::__negate function and
> associated
>      __gnu_cxx::__ops::_Iter_negate by the C++17 std::not_fn.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/bits/predefined_ops.h: Include <version>.
>

No, please don't include <version> anywhere. If you do that, it means
<functional> now defines every feature test macro in the entire library,
which makes it look like you can get smart pointers and ranges and
constexpr math all from <functional>.



>              [__cpp_lib_not_fn](__gnu_cxx::__ops::_Iter_negate): Remove.
>              [__cpp_lib_not_fn](__gnu_cxx::__ops::__negate): Remove.
>              * include/bits/stl_algo.h: Include <functional> for C++17
> and later.
>              [__cpp_lib_not_fn](__find_if_not): Use std::not_fn.
>              (std::__search, std::search(_FwdIt1, _FwdIt1, _FwdIt2,
> _FwdIt2, _BinPred)): Move...
>              * include/bits/stl_algobase.h: ...here.
>              * include/std/functional: Replace <stl_algo.h> include by
> <stl_algobase.h>.
>
> Tests still running.
>
> François
>
>
François Dumont May 24, 2023, 4:51 a.m. UTC | #2
On 22/05/2023 22:55, Jonathan Wakely wrote:
>
>
> On Mon, 22 May 2023 at 21:51, François Dumont via Libstdc++ 
> <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>
>     I was thinking that it might be nice to get rid of
>     predefined_ops.h content.
>
>     So here is a start with __negate. Drawback is that stl_algo.h has to
>     include <functional>.
>
>
> We definitely don't want that. std::not_fn could be move to its own 
> header.
>
> But I'm not sure this is a good change anyway, as we can't do it 
> unconditionally. Pre-C++17 code would still be using the 
> predefined_ops.h function objects, so we can't remove that code. And 
> we'll get template bloat from instantiating the algos twice, once with 
> the old function objects and once with std::not_fn.

True, what do you advise then ? Should I just forget about it ? 
Introduce a std::__not_fn for pre-C++17 ?

I am studying this last proposal, let me know if it is just impossible 
or a waste of time.

>
>     For now I just get rid of stl_algo.h include in
>     <functional> to rather use stl_algobase.h. But maybe it would be
>     better
>     to also isolate std::not_fn in a dedicated header file so that
>     stl_algo.h do not have to include all <functional>.
>
>          libstdc++: Replace __gnu_cxx::__ops::__negate with std::not_fn
>
>          Replace the internal __gnu_cxx::__ops::__negate function and
>     associated
>          __gnu_cxx::__ops::_Iter_negate by the C++17 std::not_fn.
>
>          libstdc++-v3/ChangeLog:
>
>                  * include/bits/predefined_ops.h: Include <version>.
>
>
> No, please don't include <version> anywhere. If you do that, it means 
> <functional> now defines every feature test macro in the entire 
> library, which makes it look like you can get smart pointers and 
> ranges and constexpr math all from <functional>.

Ok, I wasn't aware about the interest of <version>. I see now, limited 
to user code.

I'm testing only the move of std::search to stl_algobase.h to avoid 
stl_algo.h include in <functional>. I'll submit it later.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/predefined_ops.h b/libstdc++-v3/include/bits/predefined_ops.h
index e9933373ed9..8fdb11ea84b 100644
--- a/libstdc++-v3/include/bits/predefined_ops.h
+++ b/libstdc++-v3/include/bits/predefined_ops.h
@@ -30,6 +30,7 @@ 
 #ifndef _GLIBCXX_PREDEFINED_OPS_H
 #define _GLIBCXX_PREDEFINED_OPS_H	1
 
+#include <version>
 #include <bits/move.h>
 
 namespace __gnu_cxx
@@ -377,6 +378,7 @@  namespace __ops
 	  _GLIBCXX_MOVE(__comp._M_comp), __it);
     }
 
+#if !__cpp_lib_not_fn
   template<typename _Predicate>
     struct _Iter_negate
     {
@@ -400,6 +402,7 @@  namespace __ops
     inline _Iter_negate<_Predicate>
     __negate(_Iter_pred<_Predicate> __pred)
     { return _Iter_negate<_Predicate>(_GLIBCXX_MOVE(__pred._M_pred)); }
+#endif
 
 } // namespace __ops
 } // namespace __gnu_cxx
diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h
index 54695490166..849d8a59ec2 100644
--- a/libstdc++-v3/include/bits/stl_algo.h
+++ b/libstdc++-v3/include/bits/stl_algo.h
@@ -65,6 +65,10 @@ 
 #include <bits/uniform_int_dist.h>
 #endif
 
+#if __cplusplus >= 201703L
+#include <functional> // for std::not_fn.
+#endif
+
 #if _GLIBCXX_HOSTED
 # include <bits/stl_tempbuf.h>  // for _Temporary_buffer
 # if (__cplusplus <= 201103L || _GLIBCXX_USE_DEPRECATED)
@@ -110,7 +114,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		  _Predicate __pred)
     {
       return std::__find_if(__first, __last,
+#if __cpp_lib_not_fn
+			    std::not_fn(std::move(__pred)),
+#else
 			    __gnu_cxx::__ops::__negate(__pred),
+#endif
 			    std::__iterator_category(__first));
     }
 
@@ -140,54 +148,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // count
   // count_if
   // search
-
-  template<typename _ForwardIterator1, typename _ForwardIterator2,
-	   typename _BinaryPredicate>
-    _GLIBCXX20_CONSTEXPR
-    _ForwardIterator1
-    __search(_ForwardIterator1 __first1, _ForwardIterator1 __last1,
-	     _ForwardIterator2 __first2, _ForwardIterator2 __last2,
-	     _BinaryPredicate  __predicate)
-    {
-      // Test for empty ranges
-      if (__first1 == __last1 || __first2 == __last2)
-	return __first1;
-
-      // Test for a pattern of length 1.
-      _ForwardIterator2 __p1(__first2);
-      if (++__p1 == __last2)
-	return std::__find_if(__first1, __last1,
-		__gnu_cxx::__ops::__iter_comp_iter(__predicate, __first2));
-
-      // General case.
-      _ForwardIterator1 __current = __first1;
-
-      for (;;)
-	{
-	  __first1 =
-	    std::__find_if(__first1, __last1,
-		__gnu_cxx::__ops::__iter_comp_iter(__predicate, __first2));
-
-	  if (__first1 == __last1)
-	    return __last1;
-
-	  _ForwardIterator2 __p = __p1;
-	  __current = __first1;
-	  if (++__current == __last1)
-	    return __last1;
-
-	  while (__predicate(__current, __p))
-	    {
-	      if (++__p == __last2)
-		return __first1;
-	      if (++__current == __last1)
-		return __last1;
-	    }
-	  ++__first1;
-	}
-      return __first1;
-    }
-
   // search_n
 
   /**
@@ -4147,48 +4107,6 @@  _GLIBCXX_BEGIN_NAMESPACE_ALGO
 			   __gnu_cxx::__ops::__iter_equal_to_iter());
     }
 
-  /**
-   *  @brief Search a sequence for a matching sub-sequence using a predicate.
-   *  @ingroup non_mutating_algorithms
-   *  @param  __first1     A forward iterator.
-   *  @param  __last1      A forward iterator.
-   *  @param  __first2     A forward iterator.
-   *  @param  __last2      A forward iterator.
-   *  @param  __predicate  A binary predicate.
-   *  @return   The first iterator @c i in the range
-   *  @p [__first1,__last1-(__last2-__first2)) such that
-   *  @p __predicate(*(i+N),*(__first2+N)) is true for each @c N in the range
-   *  @p [0,__last2-__first2), or @p __last1 if no such iterator exists.
-   *
-   *  Searches the range @p [__first1,__last1) for a sub-sequence that
-   *  compares equal value-by-value with the sequence given by @p
-   *  [__first2,__last2), using @p __predicate to determine equality,
-   *  and returns an iterator to the first element of the
-   *  sub-sequence, or @p __last1 if no such iterator exists.
-   *
-   *  @see search(_ForwardIter1, _ForwardIter1, _ForwardIter2, _ForwardIter2)
-  */
-  template<typename _ForwardIterator1, typename _ForwardIterator2,
-	   typename _BinaryPredicate>
-    _GLIBCXX20_CONSTEXPR
-    inline _ForwardIterator1
-    search(_ForwardIterator1 __first1, _ForwardIterator1 __last1,
-	   _ForwardIterator2 __first2, _ForwardIterator2 __last2,
-	   _BinaryPredicate  __predicate)
-    {
-      // concept requirements
-      __glibcxx_function_requires(_ForwardIteratorConcept<_ForwardIterator1>)
-      __glibcxx_function_requires(_ForwardIteratorConcept<_ForwardIterator2>)
-      __glibcxx_function_requires(_BinaryPredicateConcept<_BinaryPredicate,
-	    typename iterator_traits<_ForwardIterator1>::value_type,
-	    typename iterator_traits<_ForwardIterator2>::value_type>)
-      __glibcxx_requires_valid_range(__first1, __last1);
-      __glibcxx_requires_valid_range(__first2, __last2);
-
-      return std::__search(__first1, __last1, __first2, __last2,
-			   __gnu_cxx::__ops::__iter_comp_iter(__predicate));
-    }
-
   /**
    *  @brief Search a sequence for a number of consecutive values.
    *  @ingroup non_mutating_algorithms
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index 4a6f8195d98..dd95e94f7e9 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -2150,6 +2150,53 @@  _GLIBCXX_END_NAMESPACE_ALGO
       return __result;
     }
 
+  template<typename _ForwardIterator1, typename _ForwardIterator2,
+	   typename _BinaryPredicate>
+    _GLIBCXX20_CONSTEXPR
+    _ForwardIterator1
+    __search(_ForwardIterator1 __first1, _ForwardIterator1 __last1,
+	     _ForwardIterator2 __first2, _ForwardIterator2 __last2,
+	     _BinaryPredicate  __predicate)
+    {
+      // Test for empty ranges
+      if (__first1 == __last1 || __first2 == __last2)
+	return __first1;
+
+      // Test for a pattern of length 1.
+      _ForwardIterator2 __p1(__first2);
+      if (++__p1 == __last2)
+	return std::__find_if(__first1, __last1,
+		__gnu_cxx::__ops::__iter_comp_iter(__predicate, __first2));
+
+      // General case.
+      _ForwardIterator1 __current = __first1;
+
+      for (;;)
+	{
+	  __first1 =
+	    std::__find_if(__first1, __last1,
+		__gnu_cxx::__ops::__iter_comp_iter(__predicate, __first2));
+
+	  if (__first1 == __last1)
+	    return __last1;
+
+	  _ForwardIterator2 __p = __p1;
+	  __current = __first1;
+	  if (++__current == __last1)
+	    return __last1;
+
+	  while (__predicate(__current, __p))
+	    {
+	      if (++__p == __last2)
+		return __first1;
+	      if (++__current == __last1)
+		return __last1;
+	    }
+	  ++__first1;
+	}
+      return __first1;
+    }
+
 #if __cplusplus >= 201103L
   template<typename _ForwardIterator1, typename _ForwardIterator2,
 	   typename _BinaryPredicate>
@@ -2220,6 +2267,51 @@  _GLIBCXX_END_NAMESPACE_ALGO
     }
 #endif // C++11
 
+_GLIBCXX_BEGIN_NAMESPACE_ALGO
+
+  /**
+   *  @brief Search a sequence for a matching sub-sequence using a predicate.
+   *  @ingroup non_mutating_algorithms
+   *  @param  __first1     A forward iterator.
+   *  @param  __last1      A forward iterator.
+   *  @param  __first2     A forward iterator.
+   *  @param  __last2      A forward iterator.
+   *  @param  __predicate  A binary predicate.
+   *  @return   The first iterator @c i in the range
+   *  @p [__first1,__last1-(__last2-__first2)) such that
+   *  @p __predicate(*(i+N),*(__first2+N)) is true for each @c N in the range
+   *  @p [0,__last2-__first2), or @p __last1 if no such iterator exists.
+   *
+   *  Searches the range @p [__first1,__last1) for a sub-sequence that
+   *  compares equal value-by-value with the sequence given by @p
+   *  [__first2,__last2), using @p __predicate to determine equality,
+   *  and returns an iterator to the first element of the
+   *  sub-sequence, or @p __last1 if no such iterator exists.
+   *
+   *  @see search(_ForwardIter1, _ForwardIter1, _ForwardIter2, _ForwardIter2)
+  */
+  template<typename _ForwardIterator1, typename _ForwardIterator2,
+	   typename _BinaryPredicate>
+    _GLIBCXX20_CONSTEXPR
+    inline _ForwardIterator1
+    search(_ForwardIterator1 __first1, _ForwardIterator1 __last1,
+	   _ForwardIterator2 __first2, _ForwardIterator2 __last2,
+	   _BinaryPredicate  __predicate)
+    {
+      // concept requirements
+      __glibcxx_function_requires(_ForwardIteratorConcept<_ForwardIterator1>)
+      __glibcxx_function_requires(_ForwardIteratorConcept<_ForwardIterator2>)
+      __glibcxx_function_requires(_BinaryPredicateConcept<_BinaryPredicate,
+	    typename iterator_traits<_ForwardIterator1>::value_type,
+	    typename iterator_traits<_ForwardIterator2>::value_type>)
+      __glibcxx_requires_valid_range(__first1, __last1);
+      __glibcxx_requires_valid_range(__first2, __last2);
+
+      return std::__search(__first1, __last1, __first2, __last2,
+			   __gnu_cxx::__ops::__iter_comp_iter(__predicate));
+    }
+
+_GLIBCXX_END_NAMESPACE_ALGO
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 
diff --git a/libstdc++-v3/include/std/functional b/libstdc++-v3/include/std/functional
index c7c6a5a7924..4a4b8b2b2e6 100644
--- a/libstdc++-v3/include/std/functional
+++ b/libstdc++-v3/include/std/functional
@@ -64,7 +64,7 @@ 
 #  include <vector>
 #  include <array>
 # endif
-# include <bits/stl_algo.h> // std::search
+# include <bits/stl_algobase.h> // std::search
 #endif
 #if __cplusplus >= 202002L
 # include <bits/ranges_cmp.h> // std::identity, ranges::equal_to etc.