[Bug c/92472] enhancement: 5 * constify some parameters

[Bug c/92472] enhancement: 5 * constify some parameters

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:d73d45f19180a474b1bd3af3c9cdf52da3bafc78

commit r11-74-gd73d45f19180a474b1bd3af3c9cdf52da3bafc78
Author: Martin Liska <mliska@suse.cz>
Date:   Tue Feb 4 14:55:14 2020 +0100

    Use const for some function arguments.

    gcc/ChangeLog:

    2020-02-04  Martin Liska  <mliska@suse.cz>

            PR c/92472
            * alloc-pool.h: Use const for some arguments.
            * bitmap.h: Likewise.
            * mem-stats.h: Likewise.
            * sese.h (get_entry_bb): Likewise.
            (get_exit_bb): Likewise.

[Bug c/92472] enhancement: 5 * constify some parameters

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:03f9754665b889e0988d0392db1eb35e91b97693

commit r11-76-g03f9754665b889e0988d0392db1eb35e91b97693
Author: Martin Liska <mliska@suse.cz>
Date:   Tue Feb 4 14:55:25 2020 +0100

    Use const for template argument.

    libstdc++-v3/ChangeLog:

    2020-02-04  Martin Liska  <mliska@suse.cz>

            PR c/92472
            * include/parallel/multiway_merge.h:
            Use const for _Compare template argument.

[Bug c/92472] enhancement: 5 * constify some parameters

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |marxin at gcc dot gnu.org

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
I hope all pieces are fixed now.

[Bug c/92472] enhancement: 5 * constify some parameters

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to David Binderman from comment #0)
> Message:
> 
> trunk/libstdc++-v3/include/parallel/multiway_merge.h:121:40: style:
> Parameter '__bi2' can be declared with const [constParameter]
> trunk/libstdc++-v3/include/parallel/multiway_merge.h:191:42: style:
> Parameter '__bi2' can be declared with const [constParameter]
> 
> Patch:
> 
> Index: libstdc++-v3/include/parallel/multiway_merge.h
> ===================================================================
> --- libstdc++-v3/include/parallel/multiway_merge.h  (revision 278050)
> +++ libstdc++-v3/include/parallel/multiway_merge.h  (working copy)
> @@ -118,7 +118,7 @@
>         *  @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 @@
>         *  @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 400% wrong. It doesn't even address what cppcheck is complaining about,
and cppcheck is drunk anyway. Those parameter can NOT be const, because *__b1
and *__b2 will not compile if they're const, because operator* is not const.

> All patches seemed to bootstrap ok.

But what about testing them?

[Bug c/92472] enhancement: 5 * constify some parameters

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #7 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #6)
> This is 400% wrong. It doesn't even address what cppcheck is complaining
> about, and cppcheck is drunk anyway. 

Thanks for your explanation. 
I am a bit confused by it, so further explanation sought, please. 

> Those parameter can NOT be const, because *__b1 and *__b2 will not 
> compile if they're const, because operator* is not const.

My understanding of C++, frayed somewhat since 1988, is that operator * 
being const is a different language feature to parameters being const.

Unless you know different.

> But what about testing them?

Any guidance on fixing this problem, if it is a problem at all,
would be most welcome.

[Bug c/92472] enhancement: 5 * constify some parameters

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to David Binderman from comment #7)
> (In reply to Jonathan Wakely from comment #6)
> > Those parameter can NOT be const, because *__b1 and *__b2 will not 
> > compile if they're const, because operator* is not const.
> 
> My understanding of C++, frayed somewhat since 1988, is that operator * 
> being const is a different language feature to parameters being const.

The function looks like this:

      friend bool
      operator<(const _UnguardedIterator<_RAIter, _Compare>& __bi1,
                const _UnguardedIterator<_RAIter, _Compare>& __bi2)
      {
        // Normal compare.
        return (__bi1.__comp)(*__bi1, *__bi2);
      }

i.e. it uses *__b1 which uses _UnguardedIterator::operator*

If you change __b1 to be const, you can't call non-const member functions on
that object, including _UnguardedIterator::operator*


> Any guidance on fixing this problem, if it is a problem at all,
> would be most welcome.

It's not a problem. The code should be left alone to die in peace.

[Bug c/92472] enhancement: 5 * constify some parameters

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Or in other words, of course whether a parameter can be const is separate from
whether a member function can be const. But that doesn't mean that changing a
parameter from non-const to const can't have any effect. It certainly has an
effect on which member functions you can call on the parameter.

[Bug c/92472] enhancement: 5 * constify some parameters

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #10 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #9)
> Or in other words, of course whether a parameter can be const is separate
> from whether a member function can be const. 

Agreed.

> But that doesn't mean that changing a parameter from non-const to const 
> can't have any effect. 

Double negative, but I think I know what you mean.

> It certainly has an effect on which member functions you can call on the
> parameter.

Agreed, but does it matter ? These are a bunch of comparison
functions, they AFAIK shouldn't be changing the objects they are comparing.

I am still not sure if the new code is ok or not,
and by now, TBH not bothered either way.

I gave up on C++ because of its ever growing complexity. This is another
example.

Suggest play safe and assume that JW is correct, so the proposed change to file
multiway_merge.h should be removed.

If this code is to die, should it be marked as unmaintained ?

[Bug c/92472] enhancement: 5 * constify some parameters

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to David Binderman from comment #10)
> > It certainly has an effect on which member functions you can call on the
> > parameter.
> 
> Agreed, but does it matter ? These are a bunch of comparison
> functions, they AFAIK shouldn't be changing the objects they are comparing.

You're confused. It's irrelevant whether the comparison function changes
anything, and that's not what cppcheck was complaining about anyway.

It was talking about the parameters to operator< and you didn't change that in
your patch. My comment was saying that *if* you had change what cppcheck was
complaining about, it would have been wrong for a different reason (that you
can't call operator* if the parameters are const).

> I am still not sure if the new code is ok or not,

Wasn't "This is 400% wrong" clear?

> Suggest play safe and assume that JW is correct, so the proposed change to
> file
> multiway_merge.h should be removed.

It already has been.

> If this code is to die, should it be marked as unmaintained ?

What does that mean?

[Bug c/92472] enhancement: 5 * constify some parameters

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #11)
> (In reply to David Binderman from comment #10)
> > I am still not sure if the new code is ok or not,
> 
> Wasn't "This is 400% wrong" clear?

Here's the error it caused:

/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/parallel/multiway_merge.h:273:
error: no match for 'operator<' in '__seq2 < __seq0' (operand types are
'__gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short unsigned
int*, std::vector<short unsigned int> >, std::less<short unsigned int> >' and
'__gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short unsigned
int*, std::vector<short unsigned int> >, std::less<short unsigned int> >')
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/parallel/multiway_merge.h:120:
note: candidate: 'bool
__gnu_parallel::operator<(__gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short
unsigned int*, std::vector<short unsigned int> >, std::less<short unsigned int>
>&, __gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short
unsigned int*, std::vector<short unsigned int> >, const std::less<short
unsigned int> >&)'
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/parallel/multiway_merge.h:121:
note:   no known conversion for argument 2 from
'__gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short unsigned
int*, std::vector<short unsigned int> >, std::less<short unsigned int> >' to
'__gnu_parallel::_GuardedIterator<__gnu_cxx::__normal_iterator<short unsigned
int*, std::vector<short unsigned int> >, const std::less<short unsigned int>
>&'


And the reason it's wrong is that const Foo<T, U>& and Foo<T, const U>& are
completely different things. Your patch made *one* of the parameters Foo<T,
const U>& (which is wrong because it's never called with anything matching that
type) and left the other parameter unchanged (which is even more wrong, because
now the operator can't be used to compare two objects of the same type).



( https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545214.html

[Bug c/92472] enhancement: 5 * constify some parameters

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92472

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:4cbc9d8b346b932f34828a51e8822881413951b7

commit r11-179-g4cbc9d8b346b932f34828a51e8822881413951b7
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.