Message ID | CAFk2RUYGr0tU-1Qj2tLQQjOyR2JTuuo1vWFjO81sWbcyisRwug@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | c++: Diagnose visitors with different return types for std::visit [PR95904] | expand |
On 29/09/20 01:12 +0300, Ville Voutilainen via Libstdc++ wrote: >Not completely tested yet. This does fix the problem of converting >incompatible pointer-to-function types, and thus gets rid of the suggestion >that compiling the code with -fpermissive is a possibility. There >is a special-casing for visit() for visitation of a single variant, and there >we don't even instantiate the whole table mechanism. We should really >entertain the possibility of flattening the whole visitation table; then >this check could (at least in theory) be uniformly written as just >an iteration of all table elements, which is not so convenient to do >with the nested multitable. This seems like a worthy incremental >improvement to me. > >2020-09-29 Ville Voutilainen <ville.voutilainen@gmail.com> > > PR libstdc++/95904 > * include/std/variant (__same_types): New. > (__check_visitor_result): Likewise. > (__check_visitor_results): Likewise. > (visit(_Visitor&&, _Variants&&...)): Use __check_visitor_results > in case we're visiting just one variant. > (__gen_vtable_impl</*base case*/>::_S_apply): > Check the visitor return type. >diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant >index dd8847cf829..56de78407c4 100644 >--- a/libstdc++-v3/include/std/variant >+++ b/libstdc++-v3/include/std/variant >@@ -182,7 +182,7 @@ namespace __variant > // used for raw visitation with indices passed in > struct __variant_idx_cookie { using type = __variant_idx_cookie; }; > // Used to enable deduction (and same-type checking) for std::visit: >- template<typename> struct __deduce_visit_result { }; >+ template<typename _Tp> struct __deduce_visit_result { using type = _Tp; }; > > // Visit variants that might be valueless. > template<typename _Visitor, typename... _Variants> >@@ -1017,7 +1017,22 @@ namespace __variant > > static constexpr auto > _S_apply() >- { return _Array_type{&__visit_invoke}; } >+ { >+ constexpr bool __visit_ret_type_mismatch = >+ _Array_type::__result_is_deduced::value >+ && !is_same_v<typename _Result_type::type, >+ decltype(__visit_invoke(std::declval<_Visitor>(), >+ std::declval<_Variants>()...))>; >+ if constexpr (__visit_ret_type_mismatch) >+ { >+ static_assert(!__visit_ret_type_mismatch, >+ "std::visit requires the visitor to have the same " >+ "return type for all alternatives of a variant"); >+ return __nonesuch{}; >+ } >+ else >+ return _Array_type{&__visit_invoke}; >+ } > }; > > template<typename _Result_type, typename _Visitor, typename... _Variants> >@@ -1692,6 +1707,27 @@ namespace __variant > std::forward<_Variants>(__variants)...); > } > >+ template <class _Tp, class... _Types> >+ struct __same_types : public std::bool_constant< >+ std::__and_<std::is_same<_Tp, _Types>...>::value> {}; This would be cheaper: template<typename _Tp, typename... _Types> using __same_types = typename __and_<is_same<_Tp, _Types>...>::type; Although didn't we make changes in std::variant to stop using __and_ because it exceeds the template instantiation depth for large variants? I think this is what we want: template<typename _Tp, typename... _Types> constexpr inline __same_types = (is_same_v<_Tp, _Types> && ...); is_same_v is very cheap, it uses the built-in directly, so you don't need to instantiate any class templates at all. >+ >+ template <unsigned long int _Idx, class _Visitor, class _Variant> typename not class please. >+ decltype(auto) __check_visitor_result(_Visitor&& __vis, New line after the decltype(auto) please, not in the middle of the parameter list. I'll keep staring at this to review the actual content rather than the window dressing that I've commented on above.
On Tue, 29 Sep 2020 at 14:20, Jonathan Wakely <jwakely@redhat.com> wrote: > I think this is what we want: > > template<typename _Tp, typename... _Types> > constexpr inline __same_types = (is_same_v<_Tp, _Types> && ...); > > is_same_v is very cheap, it uses the built-in directly, so you don't > need to instantiate any class templates at all. > > >+ > >+ template <unsigned long int _Idx, class _Visitor, class _Variant> > > typename not class please. > > >+ decltype(auto) __check_visitor_result(_Visitor&& __vis, > > New line after the decltype(auto) please, not in the middle of the > parameter list. Aye.
On 29/09/20 19:35 +0300, Ville Voutilainen via Libstdc++ wrote: >On Tue, 29 Sep 2020 at 14:20, Jonathan Wakely <jwakely@redhat.com> wrote: >> I think this is what we want: >> >> template<typename _Tp, typename... _Types> >> constexpr inline __same_types = (is_same_v<_Tp, _Types> && ...); >> >> is_same_v is very cheap, it uses the built-in directly, so you don't >> need to instantiate any class templates at all. >> >> >+ >> >+ template <unsigned long int _Idx, class _Visitor, class _Variant> >> >> typename not class please. >> >> >+ decltype(auto) __check_visitor_result(_Visitor&& __vis, >> >> New line after the decltype(auto) please, not in the middle of the >> parameter list. > >Aye. >diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant >index dd8847cf829..6f647d622c4 100644 >--- a/libstdc++-v3/include/std/variant >+++ b/libstdc++-v3/include/std/variant >@@ -182,7 +182,7 @@ namespace __variant > // used for raw visitation with indices passed in > struct __variant_idx_cookie { using type = __variant_idx_cookie; }; > // Used to enable deduction (and same-type checking) for std::visit: >- template<typename> struct __deduce_visit_result { }; >+ template<typename _Tp> struct __deduce_visit_result { using type = _Tp; }; > > // Visit variants that might be valueless. > template<typename _Visitor, typename... _Variants> >@@ -1017,7 +1017,22 @@ namespace __variant > > static constexpr auto > _S_apply() >- { return _Array_type{&__visit_invoke}; } >+ { >+ constexpr bool __visit_ret_type_mismatch = >+ _Array_type::__result_is_deduced::value >+ && !is_same_v<typename _Result_type::type, >+ decltype(__visit_invoke(std::declval<_Visitor>(), >+ std::declval<_Variants>()...))>; >+ if constexpr (__visit_ret_type_mismatch) >+ { >+ static_assert(!__visit_ret_type_mismatch, >+ "std::visit requires the visitor to have the same " >+ "return type for all alternatives of a variant"); >+ return __nonesuch{}; >+ } >+ else >+ return _Array_type{&__visit_invoke}; >+ } > }; > > template<typename _Result_type, typename _Visitor, typename... _Variants> >@@ -1692,6 +1707,26 @@ namespace __variant > std::forward<_Variants>(__variants)...); > } > >+ template<typename _Tp, typename... _Types> >+ constexpr inline bool __same_types = (is_same_v<_Tp, _Types> && ...); >+ >+ template <unsigned long int _Idx, typename _Visitor, typename _Variant> >+ decltype(auto) >+ __check_visitor_result(_Visitor&& __vis, _Variant&& __variant) >+ { >+ return std::forward<_Visitor>(__vis)( >+ std::get<_Idx>(std::forward<_Variant>(__variant))); Looks good, the new error is nice. git apply warns about some whitespace errors: /dev/shm/pr95904.diff:51: indent with spaces. std::get<_Idx>(std::forward<_Variant>(__variant))); /dev/shm/pr95904.diff:73: indent with spaces. { /dev/shm/pr95904.diff:77: indent with spaces. std::variant_size<remove_reference_t<_Variants>...>::value>()); /dev/shm/pr95904.diff:92: indent with spaces. std::forward<_Visitor>(__visitor), warning: 4 lines add whitespace errors. OK for trunk with those leading spaces switched to tab. Thanks!
On Sat, 3 Oct 2020 at 01:14, Jonathan Wakely <jwakely@redhat.com> wrote:
> OK for trunk with those leading spaces switched to tab.
The patch is borked, doesn't pass tests, fixing...
On Mon, 5 Oct 2020 at 01:15, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> The patch is borked, doesn't pass tests, fixing...
Unborked, ok for trunk if full testsuite passes?
2020-10-05 Ville Voutilainen <ville.voutilainen@gmail.com>
PR libstdc++/95904
* include/std/variant (__deduce_visit_result): Add a nested ::type.
(__gen_vtable_impl</*base case*/>::_S_apply):
Check the visitor return type.
(__same_types): New.
(__check_visitor_result): Likewise.
(__check_visitor_results): Likewise.
(visit(_Visitor&&, _Variants&&...)): Use __check_visitor_results
in case we're visiting just one variant.
* testsuite/20_util/variant/visit_neg.cc: Adjust.
On 05/10/20 22:35 +0300, Ville Voutilainen via Libstdc++ wrote: >On Mon, 5 Oct 2020 at 01:15, Ville Voutilainen ><ville.voutilainen@gmail.com> wrote: >> The patch is borked, doesn't pass tests, fixing... > >Unborked, ok for trunk if full testsuite passes? Assuming it has passed by now, OK. Thanks.
On 05/10/20 22:35 +0300, Ville Voutilainen via Libstdc++ wrote: >On Mon, 5 Oct 2020 at 01:15, Ville Voutilainen ><ville.voutilainen@gmail.com> wrote: >> The patch is borked, doesn't pass tests, fixing... > >Unborked, ok for trunk if full testsuite passes? > >2020-10-05 Ville Voutilainen <ville.voutilainen@gmail.com> > > PR libstdc++/95904 > * include/std/variant (__deduce_visit_result): Add a nested ::type. > (__gen_vtable_impl</*base case*/>::_S_apply): > Check the visitor return type. > (__same_types): New. > (__check_visitor_result): Likewise. > (__check_visitor_results): Likewise. > (visit(_Visitor&&, _Variants&&...)): Use __check_visitor_results > in case we're visiting just one variant. > * testsuite/20_util/variant/visit_neg.cc: Adjust. >diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant >index dd8847cf829..b32e564fd41 100644 >--- a/libstdc++-v3/include/std/variant >+++ b/libstdc++-v3/include/std/variant >@@ -182,7 +182,7 @@ namespace __variant > // used for raw visitation with indices passed in > struct __variant_idx_cookie { using type = __variant_idx_cookie; }; > // Used to enable deduction (and same-type checking) for std::visit: >- template<typename> struct __deduce_visit_result { }; >+ template<typename _Tp> struct __deduce_visit_result { using type = _Tp; }; > > // Visit variants that might be valueless. > template<typename _Visitor, typename... _Variants> >@@ -1017,7 +1017,26 @@ namespace __variant > > static constexpr auto > _S_apply() >- { return _Array_type{&__visit_invoke}; } >+ { >+ if constexpr (_Array_type::__result_is_deduced::value) >+ { >+ constexpr bool __visit_ret_type_mismatch = >+ !is_same_v<typename _Result_type::type, >+ decltype(__visit_invoke(std::declval<_Visitor>(), >+ std::declval<_Variants>()...))>; >+ if constexpr (__visit_ret_type_mismatch) >+ { >+ static_assert(!__visit_ret_type_mismatch, >+ "std::visit requires the visitor to have the same " >+ "return type for all alternatives of a variant"); >+ return __nonesuch{}; >+ } >+ else >+ return _Array_type{&__visit_invoke}; >+ } >+ else >+ return _Array_type{&__visit_invoke}; >+ } > }; > > template<typename _Result_type, typename _Visitor, typename... _Variants> >@@ -1692,6 +1711,26 @@ namespace __variant > std::forward<_Variants>(__variants)...); > } > >+ template<typename _Tp, typename... _Types> >+ constexpr inline bool __same_types = (is_same_v<_Tp, _Types> && ...); >+ >+ template <unsigned long int _Idx, typename _Visitor, typename _Variant> >+ decltype(auto) >+ __check_visitor_result(_Visitor&& __vis, _Variant&& __variant) >+ { >+ return std::__invoke(std::forward<_Visitor>(__vis), >+ std::get<_Idx>(std::forward<_Variant>(__variant))); >+ } >+ >+ template <typename _Visitor, typename _Variant, unsigned long int... _Idxs> index_sequence uses size_t not unsigned long. This parameter pack needs to be size_t... _Idxs, and the NTTP for __check_visitor_result should be size_t _Idx.
On Sat, 10 Oct 2020 at 13:52, Jonathan Wakely <jwakely@redhat.com> wrote: > index_sequence uses size_t not unsigned long. This parameter pack > needs to be size_t... _Idxs, and the NTTP for __check_visitor_result > should be size_t _Idx. Fixed in https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=02cbd79e4728319e0887ad7783297853b527bb13 after approval-over-irc.
On 08/10/2020 16:27, Jonathan Wakely via Gcc-patches wrote: > On 05/10/20 22:35 +0300, Ville Voutilainen via Libstdc++ wrote: >> On Mon, 5 Oct 2020 at 01:15, Ville Voutilainen >> <ville.voutilainen@gmail.com> wrote: >>> The patch is borked, doesn't pass tests, fixing... >> >> Unborked, ok for trunk if full testsuite passes? > > Assuming it has passed by now, OK. Thanks. Clang (with -std=c++17/20) now complains about > include/c++/11.0.0/variant:1032:10: error: no matching constructor for initialization of 'std::__nonesuch' > return __nonesuch{}; > ^ ~~ > include/c++/11.0.0/type_traits:2953:5: note: candidate constructor not viable: requires 1 argument, but 0 were provided > __nonesuch(__nonesuch const&) = delete; > ^ upon #include <variant>. (And I think legitimately so, as __nonsuch is not dependent?)
On Sat, 17 Oct 2020 at 20:30, Stephan Bergmann <sbergman@redhat.com> wrote: > Clang (with -std=c++17/20) now complains about > > > include/c++/11.0.0/variant:1032:10: error: no matching constructor for initialization of 'std::__nonesuch' > > return __nonesuch{}; > > ^ ~~ > > include/c++/11.0.0/type_traits:2953:5: note: candidate constructor not viable: requires 1 argument, but 0 were provided > > __nonesuch(__nonesuch const&) = delete; > > ^ > > upon #include <variant>. (And I think legitimately so, as __nonsuch is > not dependent?) This is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97449, I'll commit a fix today.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index dd8847cf829..56de78407c4 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -182,7 +182,7 @@ namespace __variant // used for raw visitation with indices passed in struct __variant_idx_cookie { using type = __variant_idx_cookie; }; // Used to enable deduction (and same-type checking) for std::visit: - template<typename> struct __deduce_visit_result { }; + template<typename _Tp> struct __deduce_visit_result { using type = _Tp; }; // Visit variants that might be valueless. template<typename _Visitor, typename... _Variants> @@ -1017,7 +1017,22 @@ namespace __variant static constexpr auto _S_apply() - { return _Array_type{&__visit_invoke}; } + { + constexpr bool __visit_ret_type_mismatch = + _Array_type::__result_is_deduced::value + && !is_same_v<typename _Result_type::type, + decltype(__visit_invoke(std::declval<_Visitor>(), + std::declval<_Variants>()...))>; + if constexpr (__visit_ret_type_mismatch) + { + static_assert(!__visit_ret_type_mismatch, + "std::visit requires the visitor to have the same " + "return type for all alternatives of a variant"); + return __nonesuch{}; + } + else + return _Array_type{&__visit_invoke}; + } }; template<typename _Result_type, typename _Visitor, typename... _Variants> @@ -1692,6 +1707,27 @@ namespace __variant std::forward<_Variants>(__variants)...); } + template <class _Tp, class... _Types> + struct __same_types : public std::bool_constant< + std::__and_<std::is_same<_Tp, _Types>...>::value> {}; + + template <unsigned long int _Idx, class _Visitor, class _Variant> + decltype(auto) __check_visitor_result(_Visitor&& __vis, + _Variant&& __variant) + { + return std::forward<_Visitor>(__vis)( + std::get<_Idx>(std::forward<_Variant>(__variant))); + } + + template <class _Visitor, class _Variant, unsigned long int... _Idxs> + constexpr bool __check_visitor_results(std::index_sequence<_Idxs...>) + { + return __same_types<decltype(__check_visitor_result<_Idxs>( + std::declval<_Visitor>(), + std::declval<_Variant>()))...>::value; + } + + template<typename _Visitor, typename... _Variants> constexpr decltype(auto) visit(_Visitor&& __visitor, _Variants&&... __variants) @@ -1704,8 +1740,28 @@ namespace __variant using _Tag = __detail::__variant::__deduce_visit_result<_Result_type>; - return std::__do_visit<_Tag>(std::forward<_Visitor>(__visitor), - std::forward<_Variants>(__variants)...); + if constexpr (sizeof...(_Variants) == 1) + { + constexpr bool __visit_rettypes_match = + __check_visitor_results<_Visitor, _Variants...>( + std::make_index_sequence< + std::variant_size<remove_reference_t<_Variants>...>::value>()); + if constexpr (!__visit_rettypes_match) + { + static_assert(__visit_rettypes_match, + "std::visit requires the visitor to have the same " + "return type for all alternatives of a variant"); + return; + } + else + return std::__do_visit<_Tag>( + std::forward<_Visitor>(__visitor), + std::forward<_Variants>(__variants)...); + } + else + return std::__do_visit<_Tag>( + std::forward<_Visitor>(__visitor), + std::forward<_Variants>(__variants)...); } #if __cplusplus > 201703L