On Fri, Mar 7, 2025 at 5:08 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> > > 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>. > We are also required to check `move_constructible<F>` (that I am adding as part of the patch) for empty range pack case, so this is not observable. But, I have added (sizeof...(Ts) != 0) check as I have two overloads, and this will short-circuit. > > > >> >> > >> > 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 >> > >> > >> > > >
