Re: [PATCH][libstdc++-v3 parallel mode] Tackle XXX todos: move compare to last place

[Johannes Singler <singler at ira dot uka dot de>]

Benjamin Kosnik wrote:
>> This patch fixes a small todo, and removes another (cannot remove
>> duplicates of std:: functors because they have different structure).
> 
> What about __gnu_parallel::max and __gnu_parallel::min?

They are needed for parallel "numeric" even when "algorithm" is not
included.  That's why this test fails when removing them.  Attached
patch comments this.

2009-09-24  Johannes Singler  <singler@ira.uka.de>

       * include/parallel/base.h:  Comment on presence of min/max
       duplicates.

FAIL: 26_numerics/headers/numeric/numeric_parallel_mode.cc (test for
excess errors)

>> 2009-09-23  Johannes Singler  <singler@ira.uka.de>
>>
>>         * include/parallel/algobase.h
>>         (__lexicographical_compare_switch): Move compare template
>>         parameter to the end as of others.
> 
> This part is ok, thanks.
> 
>> include/parallel/checkers.h:  // XXX Compare default template argument
> 
> I was wondering about the form here of these sorting functions. Is
> there a simpler way to arrange this? Usage is such that all known call
> sites include all three arguments. (No default argument is used.)
> Compare passed by value... 

2009-09-24  Johannes Singler  <singler@ira.uka.de>

        * include/parallel/include/parallel/checkers.h
        (is_sorted_failure, is_sorted_print_failures):  Remove (unused).
        (__is_sorted):  Remove default parameter for _Compare; remove
        inappropriate printf.

>> include/parallel/for_each_selectors.h:  // XXX move into type_traits?
> 
> _Nothing to __accumulate_binop_reduct seem to be utility bits,
> unrelated to the rest of the file. Should they be moved elsewhere?

Well, the whole file basically contains utility bits, and they are used
close together (e.g. in algo.h)

>> include/parallel/partition.h:        // XXX _Compare must have
>> first__ValueType, second__ValueType,
> 
>> include/parallel/partition.h:        // XXX binder2nd only for
>> _RAIters??
> 
> Sadly, I can't remember this one. I think this is about type
> constraints on binder2nd, or questions on the way it's instantiated.

Well, I opt for removing it then...

2009-09-24  Johannes Singler  <singler@ira.uka.de>

        * include/parallel/partition.h (__parallel_nth_element):
        Correct comment.

Please approve all patches for mainline.

-- Johannes

Index: include/parallel/checkers.h
===================================================================
--- include/parallel/checkers.h	(revision 152112)
+++ include/parallel/checkers.h	(working copy)
@@ -46,13 +46,9 @@
    * @param __comp Comparator.
    * @return @__c true if sorted, @__c false otherwise.
    */
-  // XXX Compare default template argument
   template<typename _IIter, typename _Compare>
     bool
-    __is_sorted(_IIter __begin, _IIter __end,
-              _Compare __comp
-              = std::less<typename std::iterator_traits<_IIter>::
-              _ValueType>())
+    __is_sorted(_IIter __begin, _IIter __end, _Compare __comp)
     {
       if (__begin == __end)
         return true;
@@ -64,8 +60,6 @@
         {
           if (__comp(*__current, *__recent))
             {
-              printf("__is_sorted: check failed before position %__i.\n",
-                     __position);
               return false;
             }
           __recent = __current;
@@ -75,83 +69,4 @@
       return true;
     }
 
-  /**
-   * @brief Check whether @__c [__begin, @__c __end) is sorted according to
-   * @__c __comp.
-   * Prints the position in case an unordered pair is found.
-   * @param __begin Begin iterator of sequence.
-   * @param __end End iterator of sequence.
-   * @param __first_failure The first failure is returned in this variable.
-   * @param __comp Comparator.
-   * @return @__c true if sorted, @__c false otherwise.
-   */
-  // XXX Compare default template argument
-  template<typename _IIter, typename _Compare>
-    bool
-    is_sorted_failure(_IIter __begin, _IIter __end,
-                      _IIter& __first_failure,
-                      _Compare __comp
-                      = std::less<typename std::iterator_traits<_IIter>::
-                      _ValueType>())
-    {
-      if (__begin == __end)
-        return true;
-
-      _IIter __current(__begin), __recent(__begin);
-
-      unsigned long long __position = 1;
-      for (__current++; __current != __end; __current++)
-        {
-          if (__comp(*__current, *__recent))
-            {
-              __first_failure = __current;
-              printf("__is_sorted: check failed before position %lld.\n",
-                     __position);
-              return false;
-            }
-          __recent = __current;
-          __position++;
-        }
-
-      __first_failure = __end;
-      return true;
-    }
-
-  /**
-   * @brief Check whether @__c [__begin, @__c __end) is sorted according to
-   * @__c __comp.
-   * Prints all unordered pair, including the surrounding two elements.
-   * @param __begin Begin iterator of sequence.
-   * @param __end End iterator of sequence.
-   * @param __comp Comparator.
-   * @return @__c true if sorted, @__c false otherwise.
-   */
-  template<typename _IIter, typename _Compare>
-    bool
-    // XXX Compare default template argument
-    is_sorted_print_failures(_IIter __begin, _IIter __end,
-                             _Compare __comp
-                             = std::less<typename std::iterator_traits
-                             <_IIter>::value_type>())
-    {
-      if (__begin == __end)
-        return true;
-
-      _IIter __recent(__begin);
-      bool __ok = true;
-
-      for (_IIter __pos(__begin + 1); __pos != __end; __pos++)
-        {
-          if (__comp(*__pos, *__recent))
-            {
-              printf("%ld: %d %d %d %d\n", __pos - __begin, *(__pos - 2),
-                     *(__pos- 1), *__pos, *(__pos + 1));
-              __ok = false;
-            }
-          __recent = __pos;
-        }
-      return __ok;
-    }
-}
-
 #endif /* _GLIBCXX_PARALLEL_CHECKERS_H */
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 152112)
+++ ChangeLog	(working copy)
@@ -1,5 +1,12 @@
 2009-09-24  Johannes Singler  <singler@ira.uka.de>
 
+        * include/parallel/include/parallel/checkers.h 
+        (is_sorted_failure, is_sorted_print_failures):  Remove (unused).
+        (__is_sorted):  Remove default parameter for _Compare; remove 
+        inappropriate printf.
+
+2009-09-24  Johannes Singler  <singler@ira.uka.de>
+
         * include/parallel/base.h (_EqualFromLess):  Move _Compare template
         parameter to the end as of others.
         * include/parallel/algobase.h

Index: include/parallel/base.h
===================================================================
--- include/parallel/base.h	(revision 152113)
+++ include/parallel/base.h	(working copy)
@@ -88,14 +88,11 @@
     return __i > 1 ? __i : 1; 
   }
 
-  
+
   inline bool 
   __is_parallel(const _Parallelism __p) { return __p != sequential; }
 
 
-  // XXX remove std::duplicates from here if possible,
-  // XXX but keep minimal dependencies.
-
 /** @brief Calculates the rounded-down logarithm of @__c __n for base 2.
   *  @param __n Argument.
   *  @return Returns 0 for any argument <1.
@@ -139,6 +136,8 @@
   __b = (int)((__x >>               0 ) & _CASable_mask);
 }
 
+//needed for parallel "numeric", even if "algorithm" not included
+
 /** @brief Equivalent to std::min. */
 template<typename _Tp>
   const _Tp&
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 152113)
+++ ChangeLog	(working copy)
@@ -1,5 +1,9 @@
 2009-09-24  Johannes Singler  <singler@ira.uka.de>
 
+        * include/parallel/base.h:  Comment on presence of min/max duplicates.
+
+2009-09-24  Johannes Singler  <singler@ira.uka.de>
+
         * include/parallel/algo.h: Uglify internal identifiers;
         correct line breaks.
         * include/parallel/for_each.h: Likewise.

Index: include/parallel/partition.h
===================================================================
--- include/parallel/partition.h	(revision 152113)
+++ include/parallel/partition.h	(working copy)
@@ -358,12 +358,11 @@
           std::swap(*__pivot_pos, *(__end - 1));
         __pivot_pos = __end - 1;
 
-        // XXX _Compare must have first__ValueType, second__ValueType,
-        // _ResultType
-        // _Compare == __gnu_parallel::_Lexicographic<S, int,
-        // __gnu_parallel::_Less<S, S> >
+        // _Compare must have first_value_type, second_value_type,
+        // result_type
+        // _Compare ==
+        // __gnu_parallel::_Lexicographic<S, int, __gnu_parallel::_Less<S, S> >
         // __pivot_pos == std::pair<S, int>*
-        // XXX binder2nd only for _RAIters??
         __gnu_parallel::binder2nd<_Compare, _ValueType, _ValueType, bool>
           __pred(__comp, *__pivot_pos);
 
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 152113)
+++ ChangeLog	(working copy)
@@ -1,5 +1,10 @@
 2009-09-24  Johannes Singler  <singler@ira.uka.de>
 
+        * include/parallel/partition.h (__parallel_nth_element):
+        Correct comment.
+
+2009-09-24  Johannes Singler  <singler@ira.uka.de>
+
         * include/parallel/algo.h: Uglify internal identifiers;
         correct line breaks.
         * include/parallel/for_each.h: Likewise.