[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 Patrick Palka changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #13 from Patrick Palka --- Fixed.
[Bug libstdc++/100940] views::take and views::drop should not define _S_has_simple_extra_args
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #12 from CVS Commits --- The releases/gcc-11 branch has been updated by Patrick Palka : https://gcc.gnu.org/g:f0d8d001d94166242be4387ca72fe0fc483860f1 commit r11-8613-gf0d8d001d94166242be4387ca72fe0fc483860f1 Author: Patrick Palka 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #11 from CVS Commits --- The releases/gcc-11 branch has been updated by Patrick Palka : https://gcc.gnu.org/g:bc7a522548c37daf612c2ba4c44e9ea93548ed45 commit r11-8612-gbc7a522548c37daf612c2ba4c44e9ea93548ed45 Author: Patrick Palka 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #10 from CVS Commits --- The master branch has been updated by Patrick Palka : https://gcc.gnu.org/g:2b87f3318cf6334a3a42dcf27f2fdec0fce04665 commit r12-1567-g2b87f3318cf6334a3a42dcf27f2fdec0fce04665 Author: Patrick Palka 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #9 from CVS Commits --- The master branch has been updated by Patrick Palka : https://gcc.gnu.org/g:0f4a2fb44dad6c9c140226fc19ed16109b85e6f4 commit r12-1566-g0f4a2fb44dad6c9c140226fc19ed16109b85e6f4 Author: Patrick Palka 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #8 from Patrick Palka --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #7 from TC --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 Patrick Palka changed: What|Removed |Added Target Milestone|--- |11.2 --- Comment #6 from Patrick Palka --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 Patrick Palka 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #5 from Patrick Palka --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 TC changed: What|Removed |Added CC||rs2740 at gmail dot com --- Comment #4 from TC --- (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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940
Patrick Palka 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 ---
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
- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #2 from 康桓瑋 --- (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, it will also meet > __pipe_invocable, 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100940 --- Comment #1 from 康桓瑋 --- 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, it will also meet __pipe_invocable, because _Rhs&& can be bound to const _Rhs&.
