Re: [PATCH] c++: Diagnose visitors with different return types for std::visit [PR95904]

2020-09-29 Thread Jonathan Wakely via Gcc-patches

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  

   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::_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 struct __deduce_visit_result { };
+  template struct __deduce_visit_result { using type = _Tp; };

  // Visit variants that might be valueless.
  template
@@ -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(),
+   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
@@ -1692,6 +1707,27 @@ namespace __variant
   std::forward<_Variants>(__variants)...);
}

+  template 
+struct __same_types : public std::bool_constant<
+std::__and_...>::value> {};


This would be cheaper:

  template
using __same_types = typename __and_...>::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
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 


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.



[PATCH] c++: Diagnose visitors with different return types for std::visit [PR95904]

2020-09-28 Thread Ville Voutilainen via Gcc-patches
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  

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::_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 struct __deduce_visit_result { };
+  template struct __deduce_visit_result { using type = _Tp; };
 
   // Visit variants that might be valueless.
   template
@@ -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(),
+		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
@@ -1692,6 +1707,27 @@ namespace __variant
 			   std::forward<_Variants>(__variants)...);
 }
 
+  template 
+struct __same_types : public std::bool_constant<
+std::__and_...>::value> {};
+
+  template 
+decltype(auto) __check_visitor_result(_Visitor&& __vis,
+	  _Variant&& __variant)
+{
+  return std::forward<_Visitor>(__vis)(
+std::get<_Idx>(std::forward<_Variant>(__variant)));
+}
+
+  template 
+constexpr bool __check_visitor_results(std::index_sequence<_Idxs...>)
+{
+  return __same_types(
+	std::declval<_Visitor>(),
+	std::declval<_Variant>()))...>::value;
+}
+
+
   template
 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...>::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