On Fri, 7 Mar 2025 at 15:59, Patrick Palka <ppa...@redhat.com> wrote:
> On Fri, 7 Mar 2025, Jonathan Wakely wrote: > > > > > > > On Fri, 7 Mar 2025 at 15:05, Tomasz Kamiński wrote: > > Add missing move_constructible && regular_invocable constrains on > functor type, > > for invocations of `views::zip_transform` without range arguments. > > > > libstdc++-v3/ChangeLog: > > > > * include/std/ranges (_ZipTransform::operator()): > > Add `move_constructible` and `regular_invocable` > constraints > > * testsuite/std/ranges/zip_transform/1.cc: New tests > > --- > > Tested on x86_64-linux. OK for trunk? > > > > > > I think the server hook will reject the commit message. Does `git > gcc-verify` say it's OK? > > > > The summary line has [PR111138] but that PR number is not repeated in > the ChangeLog part. > > > > There should be "<TAB>PR libstdc++/111138" either before the > "libstdc++-v3/ChangeLog:" line, or before the "* include/std/ranges" line. > > > > This policy is documented at > > https://gcc.gnu.org/contribute.html#patches > > > > "If your patch relates a bug in the compiler for which there is an > existing PR number the bug number should be stated. Use the short-form > variant [PRnnnnn] without the Bugzilla component identifier and with > > no space between 'PR' and the number. The body of the commit message > should still contain the full form (PR <component>/nnnnn) within the body > of the commit message so that Bugzilla will correctly notice the > > commit." > > > > > > libstdc++-v3/include/std/ranges | 3 ++- > > .../testsuite/std/ranges/zip_transform/1.cc | 18 > ++++++++++++++++++ > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/libstdc++-v3/include/std/ranges > b/libstdc++-v3/include/std/ranges > > index e21f5284b46..33e9926b89f 100644 > > --- a/libstdc++-v3/include/std/ranges > > +++ b/libstdc++-v3/include/std/ranges > > @@ -5332,7 +5332,8 @@ namespace views::__adaptor > > struct _ZipTransform > > { > > template<typename _Fp, typename... _Ts> > > - requires (sizeof...(_Ts) == 0) || > __detail::__can_zip_transform_view<_Fp, _Ts...> > > + requires (sizeof...(_Ts) == 0) && > move_constructible<decay_t<_Fp>> && regular_invocable<decay_t<_Fp>&> > > > > > > I would prefer parentheses here so I don't have to think about the > precedence. > > > > I think it's ((sizeof...(T) == 0) && move_cons && invoc) || > can_zip_xform, right? > > Pedantically I guess it should be > > requires (sizeof...(T) == 0 && move_cons && invoc) > || (sizeof...(T) != 0 && can_zip_xform) > > We don't want/need to check can_zip_xform if we have no range arguments > and an unsuitable functor type. > Ah yes. can_zip_xform will check sizeof...(T) > 0 because that's in the transform_view constraints, but only after checking move_constructible<F>. > > > > > Is this still missing a check that decay_t<invoke_result_t<FD&>> is an > object type? > > > > Maybe we want to create a helper concept which checks decay_t<_Fp>, e.g. > add this to __detail: > > > > template<typename _Fd> > > concept __can_xform_empty // TODO: better name? > > = move_constructible<_Fd> && regular_invocable<_Fd&> > > && is_object_v<decay_t<invoke_result_t<_Fd&>>>; > > > > And then constrain _ZipTransform::operator() with: > > > > template<typename _Fp, typename... _Ts> > > requires (sizeof...(_Ts) == 0 && > __detail::__can_xform_empty<decay_t<_Fp>>) > > || __detail::__can_zip_transform_view<_Fp, _Ts...> > > > > Or ... and maybe this is heresy ... we could overload operator() > > > > template<typename _Fp, typename... _Ts> > > requires __detail::__can_zip_transform_view<_Fp, _Ts...> > > constexpr auto > > operator() [[nodiscard]] (_Fp&& __f, _Ts&&... __ts) const > > { > > return zip_transform_view(std::forward<_Fp>(__f), > std::forward<_Ts>(__ts)...); > > } > > > > template<typename _Fp> > > requires __detail::__can_xform_empty<decay_t<_Fp>> > > constexpr auto > > operator() [[nodiscard]] (_Fp&& __f) const > > { > > return views::empty<decay_t<invoke_result_t<decay_t<_Fp>&>>>; > > } > > > > It's simpler for me to understand this way, but we would have to pay the > cost of overload resolution. So probably not a good idea. > > > > > > > > + || __detail::__can_zip_transform_view<_Fp, _Ts...> > > constexpr auto > > operator() [[nodiscard]] (_Fp&& __f, _Ts&&... __ts) const > > { > > diff --git a/libstdc++-v3/testsuite/std/ranges/zip_transform/1.cc > b/libstdc++-v3/testsuite/std/ranges/zip_transform/1.cc > > index 20abdcba0f8..67839261cc7 100644 > > --- a/libstdc++-v3/testsuite/std/ranges/zip_transform/1.cc > > +++ b/libstdc++-v3/testsuite/std/ranges/zip_transform/1.cc > > @@ -9,6 +9,20 @@ > > namespace ranges = std::ranges; > > namespace views = std::views; > > > > +template<typename T> > > +concept can_zip_transform = requires (T t) { > > + views::zip_transform(std::forward<T>(t)); > > +}; > > + > > +static_assert(!can_zip_transform<int>); > > + > > +struct NonMovable { > > + NonMovable(NonMovable&&) = delete; > > +}; > > + > > +static_assert(!can_zip_transform<NonMovable>); > > +static_assert(!can_zip_transform<NonMovable&>); > > + > > constexpr bool > > test01() > > { > > @@ -46,6 +60,10 @@ test01() > > VERIFY( ranges::size(z3) == 3 ); > > VERIFY( ranges::equal(z3, (int[]){3, 6, 9}) ); > > > > + auto z4 = views::zip_transform([] () { return 1; }); > > + VERIFY( ranges::size(z4) == 0 ); > > + static_assert( > std::same_as<ranges::range_value_t<decltype(z4)>, int> ); > > + > > return true; > > } > > > > -- > > 2.48.1 > > > > > >
