[Bug libstdc++/100940] New: views::take and views::drop should not define _S_has_simple_extra_args

[Bug libstdc++/100940] New: views::take and views::drop should not define _S_has_simple_extra_args

[hewillk at gmail dot com]

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

            Bug ID: 100940
           Summary: views::take and views::drop should not define
                    _S_has_simple_extra_args
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hewillk at gmail dot com
  Target Milestone: ---

For view::take and views::drop, we need to perfectly forward its incoming arg
in some uncommon situations:

#include <ranges>

struct Five {
  operator int() && { return 5; }
} five;

extern int x[10];
auto r = x | std::views::take(five);

https://godbolt.org/z/MEsssWGEh

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[hewillk at gmail dot com]

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

--- Comment #1 from 康桓瑋 <hewillk at gmail dot com> ---
It seems that before r12-1184, the solution to PR100577 can be simply to remove
const && delete operator(). The reason is that const & and && overloads
themselves are constrained, and if _Rhs meets __pipe_invocable<const _Lhs&,
const _Rhs&, _Range>, it will also meet __pipe_invocable<const _Lhs&, _Rhs,
_Range>, because _Rhs&& can be bound to const _Rhs&.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[hewillk at gmail dot com]

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

--- Comment #2 from 康桓瑋 <hewillk at gmail dot com> ---
(In reply to 康桓瑋 from comment #1)
> It seems that before r12-1184, the solution to PR100577 can be simply to
> remove const && delete operator(). The reason is that const & and &&
> overloads themselves are constrained, and if _Rhs meets
> __pipe_invocable<const _Lhs&, const _Rhs&, _Range>, it will also meet
> __pipe_invocable<const _Lhs&, _Rhs, _Range>, because _Rhs&& can be bound to
> const _Rhs&.

Oops, I missed the split_view example, so the delete operator() is very
necessary.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
                 CC|                            |ppalka at gcc dot gnu.org
   Last reconfirmed|                            |2021-06-07
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Good point, confirmed.  Though I'm not sure if perfect forwarding here is
strictly necessary to fix this testcase.  Perhaps the _S_has_simple_extra_args
versions of _Partial should be forwarding the bound arguments as prvalues
instead of as const lvalues?

The following

--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -933,10 +933,10 @@ namespace views::__adaptor
       { }

       template<typename _Range>
-       requires __adaptor_invocable<_Adaptor, _Range, const _Arg&>
+       requires __adaptor_invocable<_Adaptor, _Range, _Arg>
        constexpr auto
        operator()(_Range&& __r) const
-       { return _Adaptor{}(std::forward<_Range>(__r), _M_arg); }
+       { return _Adaptor{}(std::forward<_Range>(__r),
static_cast<_Arg>(_M_arg)); }

       static constexpr bool _S_has_simple_call_op = true;
     };

would fix your example.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[rs2740 at gmail dot com]

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

TC <rs2740 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rs2740 at gmail dot com

--- Comment #4 from TC <rs2740 at gmail dot com> ---
(In reply to Patrick Palka from comment #3)
> Good point, confirmed.  Though I'm not sure if perfect forwarding here is
> strictly necessary to fix this testcase.  Perhaps the
> _S_has_simple_extra_args versions of _Partial should be forwarding the bound
> arguments as prvalues instead of as const lvalues?

It's pretty easy to come up with counterexamples that don't work (for example,
the type might be move-only).

It may be better to limit the "simple" case for take/drop to when the argument
type is integer-like; that's like 99% of uses anyway. Contrived examples gets
the perfect forwarding fun but that's fine.

Similarly, it might be a good idea to restrict the "simple" case for the other
adaptors a bit - perhaps to the case where the predicate is trivially copyable,
which should still give good diagnostic for a lot of uses, but avoids a
performance hit if the function object at issue is like...std::function.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

--- Comment #5 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to TC from comment #4)
> (In reply to Patrick Palka from comment #3)
> > Good point, confirmed.  Though I'm not sure if perfect forwarding here is
> > strictly necessary to fix this testcase.  Perhaps the
> > _S_has_simple_extra_args versions of _Partial should be forwarding the bound
> > arguments as prvalues instead of as const lvalues?
> 
> It's pretty easy to come up with counterexamples that don't work (for
> example, the type might be move-only).
> 
> It may be better to limit the "simple" case for take/drop to when the
> argument type is integer-like; that's like 99% of uses anyway. Contrived
> examples gets the perfect forwarding fun but that's fine.
> 
> Similarly, it might be a good idea to restrict the "simple" case for the
> other adaptors a bit - perhaps to the case where the predicate is trivially
> copyable, which should still give good diagnostic for a lot of uses, but
> avoids a performance hit if the function object at issue is
> like...std::function.

That makes sense to me.  Implementation wise I guess this would mean
parameterizing the _S_has_simple_extra_args flag by the actual types of the
extra arguments.   And I suppose we could also use this to declare some partial
applications of split to be simple, e.g. when the pattern argument is a scalar
or a view, and get good diagnostics for split in these cases.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |ppalka at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.2

--- Comment #6 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Patch at: https://gcc.gnu.org/pipermail/libstdc++/2021-June/052736.html

The patch disables perfect forwarding for take/drop only when the argument is
integer-like (fixing the reported correctness issue), and also disables perfect
forwarding for split when the argument is a scalar or a copy-constructible view
(safely restoring good diagnostics for these cases).

For the other adaptors, we still unconditionally disable perfect forwarding
call wrapper semantics.  I'm not sure if the performance/diagnostic tradeoff is
worth it to enable perfect forwarding semantics when the function object is
non-trivial.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[rs2740 at gmail dot com]

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

--- Comment #7 from TC <rs2740 at gmail dot com> ---
(In reply to Patrick Palka from comment #6)
> 
> For the other adaptors, we still unconditionally disable perfect forwarding
> call wrapper semantics.  I'm not sure if the performance/diagnostic tradeoff
> is worth it to enable perfect forwarding semantics when the function object
> is non-trivial.

Personally, I'd happily pay some diagnostic complexity when I get things wrong
if that means I get better performance when I get things right. Good
diagnostics only matter when my code is broken, while performance matters when
my code is working. The former should (hopefully) be a transient condition.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

--- Comment #8 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to TC from comment #7)
> (In reply to Patrick Palka from comment #6)
> > 
> > For the other adaptors, we still unconditionally disable perfect forwarding
> > call wrapper semantics.  I'm not sure if the performance/diagnostic tradeoff
> > is worth it to enable perfect forwarding semantics when the function object
> > is non-trivial.
> 
> Personally, I'd happily pay some diagnostic complexity when I get things
> wrong if that means I get better performance when I get things right. Good
> diagnostics only matter when my code is broken, while performance matters
> when my code is working. The former should (hopefully) be a transient
> condition.

Sounds good; a followup patch that implements this is at
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572829.html

The patch limits the "simple" forwarding case to only trivially copyable extra
arguments, and does so for all adaptors.  So
take_while/drop_while/filter/transform(std::function) as well as
split(non_triv_copyable_view) would be considered non-simple under this
restriction.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:0f4a2fb44dad6c9c140226fc19ed16109b85e6f4

commit r12-1566-g0f4a2fb44dad6c9c140226fc19ed16109b85e6f4
Author: Patrick Palka <ppalka@redhat.com>
Date:   Thu Jun 17 09:46:04 2021 -0400

    libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]

    The _S_has_simple_extra_args mechanism is used to simplify forwarding
    of range adaptor's extra arguments when perfect forwarding call wrapper
    semantics isn't required for correctness, on a per-adaptor basis.
    Both views::take and views::drop are flagged as such, but it turns out
    perfect forwarding semantics are needed for these adaptors in some
    contrived cases, e.g. when their extra argument is a move-only class
    that's implicitly convertible to an integral type.

    To fix this, we could just clear the flag for views::take/drop as with
    views::split, but that'd come at the cost of acceptable diagnostics
    for ill-formed uses of these adaptors (see PR100577).

    This patch instead allows adaptors to parameterize their
    _S_has_simple_extra_args flag according the types of the captured extra
    arguments, so that we could conditionally disable perfect forwarding
    semantics only when the types of the extra arguments permit it.  We
    then use this finer-grained mechanism to safely disable perfect
    forwarding semantics for views::take/drop when the extra argument is
    integer-like, rather than incorrectly always disabling it.  Similarly,
    for views::split, rather than always enabling perfect forwarding
    semantics we now safely disable it when the extra argument is a scalar
    or a view, and recover good diagnostics for these common cases.

            PR libstdc++/100940

    libstdc++-v3/ChangeLog:

            * include/std/ranges (__adaptor::_RangeAdaptor): Document the
            template form of _S_has_simple_extra_args.
            (__adaptor::__adaptor_has_simple_extra_args): Add _Args template
            parameter pack.  Try to treat _S_has_simple_extra_args as a
            variable template parameterized by _Args.
            (__adaptor::_Partial): Pass _Arg/_Args to the constraint
            __adaptor_has_simple_extra_args.
            (views::_Take::_S_has_simple_extra_args): Templatize according
            to the type of the extra argument.
            (views::_Drop::_S_has_simple_extra_args): Likewise.
            (views::_Split::_S_has_simple_extra_args): Define.
            * testsuite/std/ranges/adaptors/100577.cc (test01, test02):
            Adjust after changes to _S_has_simple_extra_args mechanism.
            (test03): Define.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:2b87f3318cf6334a3a42dcf27f2fdec0fce04665

commit r12-1567-g2b87f3318cf6334a3a42dcf27f2fdec0fce04665
Author: Patrick Palka <ppalka@redhat.com>
Date:   Thu Jun 17 09:46:07 2021 -0400

    libstdc++: Non-triv-copyable extra args aren't simple [PR100940]

    This force-enables perfect forwarding call wrapper semantics whenever
    the extra arguments of a partially applied range adaptor aren't all
    trivially copyable, so as to avoid incurring unnecessary copies of
    potentially expensive-to-copy objects (such as std::function objects)
    when invoking the adaptor.

            PR libstdc++/100940

    libstdc++-v3/ChangeLog:

            * include/std/ranges (__adaptor::_Partial): For the "simple"
            forwarding partial specializations, also require that
            the extra arguments are trivially copyable.
            * testsuite/std/ranges/adaptors/100577.cc (test04): New test.

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Patrick Palka
<ppalka@gcc.gnu.org>:

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

commit r11-8612-gbc7a522548c37daf612c2ba4c44e9ea93548ed45
Author: Patrick Palka <ppalka@redhat.com>
Date:   Thu Jun 17 09:46:04 2021 -0400

    libstdc++: Refine range adaptors' "simple extra args" mechanism [PR100940]

    The _S_has_simple_extra_args mechanism is used to simplify forwarding
    of range adaptor's extra arguments when perfect forwarding call wrapper
    semantics isn't required for correctness, on a per-adaptor basis.
    Both views::take and views::drop are flagged as such, but it turns out
    perfect forwarding semantics are needed for these adaptors in some
    contrived cases, e.g. when their extra argument is a move-only class
    that's implicitly convertible to an integral type.

    To fix this, we could just clear the flag for views::take/drop as with
    views::split, but that'd come at the cost of acceptable diagnostics
    for ill-formed uses of these adaptors (see PR100577).

    This patch instead allows adaptors to parameterize their
    _S_has_simple_extra_args flag according the types of the captured extra
    arguments, so that we could conditionally disable perfect forwarding
    semantics only when the types of the extra arguments permit it.  We
    then use this finer-grained mechanism to safely disable perfect
    forwarding semantics for views::take/drop when the extra argument is
    integer-like, rather than incorrectly always disabling it.  Similarly,
    for views::split, rather than always enabling perfect forwarding
    semantics we now safely disable it when the extra argument is a scalar
    or a view, and recover good diagnostics for these common cases.

            PR libstdc++/100940

    libstdc++-v3/ChangeLog:

            * include/std/ranges (__adaptor::_RangeAdaptor): Document the
            template form of _S_has_simple_extra_args.
            (__adaptor::__adaptor_has_simple_extra_args): Add _Args template
            parameter pack.  Try to treat _S_has_simple_extra_args as a
            variable template parameterized by _Args.
            (__adaptor::_Partial): Pass _Arg/_Args to the constraint
            __adaptor_has_simple_extra_args.
            (views::_Take::_S_has_simple_extra_args): Templatize according
            to the type of the extra argument.
            (views::_Drop::_S_has_simple_extra_args): Likewise.
            (views::_Split::_S_has_simple_extra_args): Define.
            * testsuite/std/ranges/adaptors/100577.cc (test01, test02):
            Adjust after changes to _S_has_simple_extra_args mechanism.
            (test03): Define.

    (cherry picked from commit 0f4a2fb44dad6c9c140226fc19ed16109b85e6f4)

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Patrick Palka
<ppalka@gcc.gnu.org>:

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

commit r11-8613-gf0d8d001d94166242be4387ca72fe0fc483860f1
Author: Patrick Palka <ppalka@redhat.com>
Date:   Thu Jun 17 09:46:07 2021 -0400

    libstdc++: Non-triv-copyable extra args aren't simple [PR100940]

    This force-enables perfect forwarding call wrapper semantics whenever
    the extra arguments of a partially applied range adaptor aren't all
    trivially copyable, so as to avoid incurring unnecessary copies of
    potentially expensive-to-copy objects (such as std::function objects)
    when invoking the adaptor.

            PR libstdc++/100940

    libstdc++-v3/ChangeLog:

            * include/std/ranges (__adaptor::_Partial): For the "simple"
            forwarding partial specializations, also require that
            the extra arguments are trivially copyable.
            * testsuite/std/ranges/adaptors/100577.cc (test04): New test.

    (cherry picked from commit 2b87f3318cf6334a3a42dcf27f2fdec0fce04665)

[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Fixed.