Unexpected greedy ops failure

Unexpected greedy ops failure

[François Dumont]

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

With the patch proposed in: 
https://gcc.gnu.org/pipermail/libstdc++/2021-August/053009.html to 
extend some _GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS I have 2 tests 
in failure:

Running target unix/-D_GLIBCXX_DEBUG
Running 
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp 
...
FAIL: 23_containers/deque/types/1.cc (test for excess errors)
FAIL: 23_containers/vector/types/1.cc (test for excess errors)

The error is:

/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/helper_functions.h:96: 
error: ambiguous overload for 'operator-' in '__rhs - __lhs' (operand 
types are 'std::move_iterator<greedy_ops::X*>' and 
'std::move_iterator<greedy_ops::X*>')
In file included from 
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:24:
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/util/testsuite_greedy_ops.h:48: 
note: candidate: 'greedy_ops::X greedy_ops::operator-(T, T) [with T = 
std::move_iterator<greedy_ops::X*>]'
In file included from 
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:67,
                  from 
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:60,
                  from 
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:23:
/home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_iterator.h:1692: 
note: candidate: 'constexpr decltype ((__x.base() - __y.base())) 
std::operator-(const std::move_iterator<_IteratorL>&, const 
std::move_iterator<_IteratorR>&) [with _IteratorL = gree
dy_ops::X*; _IteratorR = greedy_ops::X*; decltype ((__x.base() - 
__y.base())) = long int]'
compiler exited with status 1
FAIL: 23_containers/vector/types/1.cc (test for excess errors)

I really don"t understand the link between the failures and this patch.

But at the same time I am surprised that there is no definition for 
operator-(const move_iterator&, const move_iterator&) in the library 
(like in attached patch). With this change the failure vanishes.

Moreover is there a reason for having added several operators as normal 
std namespace operators rather than inline friend like in my patch ? I 
know we already talk about it but I don't remember if the reply was that 
we cannot do it or I simply forgot to propose the patch to do so.

Thanks,

François


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

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index a8986d11dfe..4c46fd43a8d 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1534,6 +1534,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return std::move(_M_current[__n]); }
 #endif
 
+      [[__nodiscard__]]
+      friend _GLIBCXX17_CONSTEXPR difference_type
+      operator-(const move_iterator& __x, const move_iterator& __y)
+      { return __x.base() - __y.base(); }
+
 #if __cplusplus > 201703L && __cpp_lib_concepts
       template<sentinel_for<_Iterator> _Sent>
 	[[nodiscard]]

Re: Unexpected greedy ops failure

[Jonathan Wakely]

On Wed, 11 Aug 2021 at 14:23, François Dumont via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> With the patch proposed in:
> https://gcc.gnu.org/pipermail/libstdc++/2021-August/053009.html to
> extend some _GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS I have 2 tests
> in failure:
>
> Running target unix/-D_GLIBCXX_DEBUG
> Running
> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> ...
> FAIL: 23_containers/deque/types/1.cc (test for excess errors)
> FAIL: 23_containers/vector/types/1.cc (test for excess errors)
>
> The error is:
>
> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/helper_functions.h:96:
> error: ambiguous overload for 'operator-' in '__rhs - __lhs' (operand
> types are 'std::move_iterator<greedy_ops::X*>' and
> 'std::move_iterator<greedy_ops::X*>')
> In file included from
> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:24:
> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/util/testsuite_greedy_ops.h:48:
> note: candidate: 'greedy_ops::X greedy_ops::operator-(T, T) [with T =
> std::move_iterator<greedy_ops::X*>]'
> In file included from
> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:67,
>                   from
> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:60,
>                   from
> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:23:
> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_iterator.h:1692:
> note: candidate: 'constexpr decltype ((__x.base() - __y.base()))
> std::operator-(const std::move_iterator<_IteratorL>&, const
> std::move_iterator<_IteratorR>&) [with _IteratorL = gree
> dy_ops::X*; _IteratorR = greedy_ops::X*; decltype ((__x.base() -
> __y.base())) = long int]'
> compiler exited with status 1
> FAIL: 23_containers/vector/types/1.cc (test for excess errors)
>
> I really don"t understand the link between the failures and this patch.

The same tests fail with -std=gnu++20 since Jason made a fix to G++. I
haven't decided yet whether we should just disable those tests for
C++20, or do something else.

Basically, the "greedy" operators on those tests are bad, and nobody
should write code like that (C++20 concepts make it unnecessary to do
it anyway).


> But at the same time I am surprised that there is no definition for
> operator-(const move_iterator&, const move_iterator&) in the library
> (like in attached patch). With this change the failure vanishes.

See line 1689 in <bits/stl_iterator.h>


>
> Moreover is there a reason for having added several operators as normal
> std namespace operators rather than inline friend like in my patch ? I
> know we already talk about it but I don't remember if the reply was that
> we cannot do it or I simply forgot to propose the patch to do so.

The standard declares that operator- in namespace std, not as a
friend, see the synopsis in [iterator.synopsis]. That has observable
semantic effects, e.g. it's possible to call it with:

struct X {
  operator std::move_iterator<int*>() const;
};
X x;
auto d = x - x;

If the function is only defined as a friend, then it can only be found
by ADL, and this example won't compile.

I suppose we could add the hidden friend *and* keep the generic
overload at namespace scope. That would meet the requirements of the
standard. I think we would still have the problem with greedy
operators for comparing related but not identical types, e.g.
move_iterator<int*> and move_iterator<const int*>.

Re: Unexpected greedy ops failure

[François Dumont]

On 11/08/21 3:32 pm, Jonathan Wakely wrote:
> On Wed, 11 Aug 2021 at 14:23, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> With the patch proposed in:
>> https://gcc.gnu.org/pipermail/libstdc++/2021-August/053009.html to
>> extend some _GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS I have 2 tests
>> in failure:
>>
>> Running target unix/-D_GLIBCXX_DEBUG
>> Running
>> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>> ...
>> FAIL: 23_containers/deque/types/1.cc (test for excess errors)
>> FAIL: 23_containers/vector/types/1.cc (test for excess errors)
>>
>> The error is:
>>
>> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/debug/helper_functions.h:96:
>> error: ambiguous overload for 'operator-' in '__rhs - __lhs' (operand
>> types are 'std::move_iterator<greedy_ops::X*>' and
>> 'std::move_iterator<greedy_ops::X*>')
>> In file included from
>> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:24:
>> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/util/testsuite_greedy_ops.h:48:
>> note: candidate: 'greedy_ops::X greedy_ops::operator-(T, T) [with T =
>> std::move_iterator<greedy_ops::X*>]'
>> In file included from
>> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:67,
>>                    from
>> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:60,
>>                    from
>> /home/fdt/dev/gcc/git/libstdc++-v3/testsuite/23_containers/vector/types/1.cc:23:
>> /home/fdt/dev/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_iterator.h:1692:
>> note: candidate: 'constexpr decltype ((__x.base() - __y.base()))
>> std::operator-(const std::move_iterator<_IteratorL>&, const
>> std::move_iterator<_IteratorR>&) [with _IteratorL = gree
>> dy_ops::X*; _IteratorR = greedy_ops::X*; decltype ((__x.base() -
>> __y.base())) = long int]'
>> compiler exited with status 1
>> FAIL: 23_containers/vector/types/1.cc (test for excess errors)
>>
>> I really don"t understand the link between the failures and this patch.
> The same tests fail with -std=gnu++20 since Jason made a fix to G++. I
> haven't decided yet whether we should just disable those tests for
> C++20, or do something else.

Ok, so I rebuilt the compiler and I do not see this strange side effect 
anymore. My patch on the other thread is fully tested, in _GLIBCXX_DEBUG 
mode too.

>
> Basically, the "greedy" operators on those tests are bad, and nobody
> should write code like that (C++20 concepts make it unnecessary to do
> it anyway).
>
>
>> But at the same time I am surprised that there is no definition for
>> operator-(const move_iterator&, const move_iterator&) in the library
>> (like in attached patch). With this change the failure vanishes.
> See line 1689 in <bits/stl_iterator.h>
Yes, I saw this one but I meant the basic operator- with both operands 
of the same type like the one in the proposed patch.
>
>> Moreover is there a reason for having added several operators as normal
>> std namespace operators rather than inline friend like in my patch ? I
>> know we already talk about it but I don't remember if the reply was that
>> we cannot do it or I simply forgot to propose the patch to do so.
> The standard declares that operator- in namespace std, not as a
> friend, see the synopsis in [iterator.synopsis]. That has observable
> semantic effects, e.g. it's possible to call it with:
>
> struct X {
>    operator std::move_iterator<int*>() const;
> };
> X x;
> auto d = x - x;
>
> If the function is only defined as a friend, then it can only be found
> by ADL, and this example won't compile.
>
> I suppose we could add the hidden friend *and* keep the generic
> overload at namespace scope. That would meet the requirements of the
> standard. I think we would still have the problem with greedy
> operators for comparing related but not identical types, e.g.
> move_iterator<int*> and move_iterator<const int*>.

Maybe we should cause it looks like it boosts a little bit the 
compilation time.

Before I rebuilt the compiler the difference was noticeable from a human 
perspective (but I didn't measure it at that time). Now I measured it 
and with this testsuite_files:

23_containers/deque/types/1.cc
23_containers/vector/types/1.cc

time make check-debug:

real    0m20.406s
user    0m19.522s
sys    0m0.905s

with the additional operator:

real    0m20.391s
user    0m19.689s
sys    0m0.722s

The diff is not huge but with the additional operator the compiler can 
pick up this perfect match without considering all the others, no ?