Re: [PATCH 4/4] Use const for template argument.

Re: [PATCH 4/4] Use const for template argument.

[Jonathan Wakely]

On 04/02/20 14:55 +0100, Martin Liska wrote:
>diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
>index 983c7b2bd9a..97a9ce0feb7 100644
>--- a/libstdc++-v3/include/parallel/multiway_merge.h
>+++ b/libstdc++-v3/include/parallel/multiway_merge.h
>@@ -118,7 +118,7 @@ namespace __gnu_parallel
>        *  @return @c true if less. */
>       friend bool
>       operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
>-               _GuardedIterator<_RAIter, _Compare>& __bi2)
>+               _GuardedIterator<_RAIter, const _Compare>& __bi2)
>       {
>        if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
>          return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
>@@ -188,7 +188,7 @@ namespace __gnu_parallel
>        *  @return @c true if less. */
>       friend bool
>       operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
>-               _UnguardedIterator<_RAIter, _Compare>& __bi2)
>+               _UnguardedIterator<_RAIter, const _Compare>& __bi2)
>       {
>        // Normal compare.
>        return (__bi1.__comp)(*__bi1, *__bi2);


This is completely bogus, please revert.

The cppcheck warning is saying that it could be:

     const _UnguardedIterator<_RAIter, _Compare>&

which is completely different from:

     _UnguardedIterator<_RAIter, const _Compare>&

Also both parameters of operator< should have been changed, not just
one, and operator<= should have the same change.

But cppcheck is completely wrong anyway. The operator* member of
_GuardedIterator and _UnguardedIterator is not const, so trying to
dereference *__b1 and *__b2 would fail.

Nack nack nack.

Re: [PATCH 4/4] Use const for template argument.

[Jonathan Wakely]

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

On 06/05/20 11:01 +0100, Jonathan Wakely wrote:
>On 04/02/20 14:55 +0100, Martin Liska wrote:
>>diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
>>index 983c7b2bd9a..97a9ce0feb7 100644
>>--- a/libstdc++-v3/include/parallel/multiway_merge.h
>>+++ b/libstdc++-v3/include/parallel/multiway_merge.h
>>@@ -118,7 +118,7 @@ namespace __gnu_parallel
>>       *  @return @c true if less. */
>>      friend bool
>>      operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
>>-               _GuardedIterator<_RAIter, _Compare>& __bi2)
>>+               _GuardedIterator<_RAIter, const _Compare>& __bi2)
>>      {
>>       if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
>>         return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
>>@@ -188,7 +188,7 @@ namespace __gnu_parallel
>>       *  @return @c true if less. */
>>      friend bool
>>      operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
>>-               _UnguardedIterator<_RAIter, _Compare>& __bi2)
>>+               _UnguardedIterator<_RAIter, const _Compare>& __bi2)
>>      {
>>       // Normal compare.
>>       return (__bi1.__comp)(*__bi1, *__bi2);
>
>
>This is completely bogus, please revert.
>
>The cppcheck warning is saying that it could be:
>
>    const _UnguardedIterator<_RAIter, _Compare>&
>
>which is completely different from:
>
>    _UnguardedIterator<_RAIter, const _Compare>&
>
>Also both parameters of operator< should have been changed, not just
>one, and operator<= should have the same change.
>
>But cppcheck is completely wrong anyway. The operator* member of
>_GuardedIterator and _UnguardedIterator is not const, so trying to
>dereference *__b1 and *__b2 would fail.
>
>Nack nack nack.

Here's a correct fix for the cppcheck complaint.

Tested powerpc64le-linux, 'make check check-parallel', committed to
master.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4339 bytes --]

commit 4cbc9d8b346b932f34828a51e8822881413951b7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 7 21:43:49 2020 +0100

    libstdc++: Make relational operators work with const guarded iterators (PR 92472)
    
    This is a correct fix for the incorrect cppcheck suggestion to make
    these parameters const. In order to that, the dereference operators need
    to be const. The conversions to the underlying iterator can be const
    too.
    
            PR c/92472
            * include/parallel/multiway_merge.h (_GuardedIterator::operator*)
            (_GuardedIterator::operator _RAIter, _UnguardedIterator::operator*)
            (_UnguardedIterator::operator _RAIter): Add const qualifier.
            (operator<(_GuardedIterator&, _GuardedIterator&)
            (operator<=(_GuardedIterator&, _GuardedIterator&)
            (operator<(_UnguardedIterator&, _UnguardedIterator&)
            (operator<=(_UnguardedIterator&, _UnguardedIterator&): Change
            parameters to const references.

diff --git a/libstdc++-v3/include/parallel/multiway_merge.h b/libstdc++-v3/include/parallel/multiway_merge.h
index 983c7b2bd9a..52a8b2ca9e7 100644
--- a/libstdc++-v3/include/parallel/multiway_merge.h
+++ b/libstdc++-v3/include/parallel/multiway_merge.h
@@ -104,12 +104,12 @@ namespace __gnu_parallel
       /** @brief Dereference operator.
       *  @return Referenced element. */
       typename std::iterator_traits<_RAIter>::value_type&
-      operator*()
+      operator*() const
       { return *_M_current; }
 
       /** @brief Convert to wrapped iterator.
       *  @return Wrapped iterator. */
-      operator _RAIter()
+      operator _RAIter() const
       { return _M_current; }
 
       /** @brief Compare two elements referenced by guarded iterators.
@@ -117,8 +117,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c true if less. */
       friend bool
-      operator<(_GuardedIterator<_RAIter, _Compare>& __bi1,
-		_GuardedIterator<_RAIter, _Compare>& __bi2)
+      operator<(const _GuardedIterator<_RAIter, _Compare>& __bi1,
+		const _GuardedIterator<_RAIter, _Compare>& __bi2)
       {
 	if (__bi1._M_current == __bi1._M_end)       // __bi1 is sup
 	  return __bi2._M_current == __bi2._M_end;  // __bi2 is not sup
@@ -132,8 +132,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c True if less equal. */
       friend bool
-      operator<=(_GuardedIterator<_RAIter, _Compare>& __bi1,
-		 _GuardedIterator<_RAIter, _Compare>& __bi2)
+      operator<=(const _GuardedIterator<_RAIter, _Compare>& __bi1,
+		 const _GuardedIterator<_RAIter, _Compare>& __bi2)
       {
 	if (__bi2._M_current == __bi2._M_end)       // __bi1 is sup
 	  return __bi1._M_current != __bi1._M_end;  // __bi2 is not sup
@@ -174,12 +174,12 @@ namespace __gnu_parallel
       /** @brief Dereference operator.
       *  @return Referenced element. */
       typename std::iterator_traits<_RAIter>::value_type&
-      operator*()
+      operator*() const
       { return *_M_current; }
 
       /** @brief Convert to wrapped iterator.
       *  @return Wrapped iterator. */
-      operator _RAIter()
+      operator _RAIter() const
       { return _M_current; }
 
       /** @brief Compare two elements referenced by unguarded iterators.
@@ -187,8 +187,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c true if less. */
       friend bool
-      operator<(_UnguardedIterator<_RAIter, _Compare>& __bi1,
-		_UnguardedIterator<_RAIter, _Compare>& __bi2)
+      operator<(const _UnguardedIterator<_RAIter, _Compare>& __bi1,
+		const _UnguardedIterator<_RAIter, _Compare>& __bi2)
       {
 	// Normal compare.
 	return (__bi1.__comp)(*__bi1, *__bi2);
@@ -199,8 +199,8 @@ namespace __gnu_parallel
        *  @param __bi2 Second iterator.
        *  @return @c True if less equal. */
       friend bool
-      operator<=(_UnguardedIterator<_RAIter, _Compare>& __bi1,
-		 _UnguardedIterator<_RAIter, _Compare>& __bi2)
+      operator<=(const _UnguardedIterator<_RAIter, _Compare>& __bi1,
+		 const _UnguardedIterator<_RAIter, _Compare>& __bi2)
       {
 	// Normal compare.
 	return !(__bi1.__comp)(*__bi2, *__bi1);