From f00470dc02917ca40b69d6359df0d61a9fa125b0 Mon Sep 17 00:00:00 2001
From: Mickey Rose <mickey.rose@seznam.cz>
Date: Tue, 13 Dec 2016 15:50:12 +0100
Subject: [PATCH] simplify mapnik::value conversion rules

- fixes #3570

- avoids recursive exception-specification on value constructor
  by only constructing a temporary for arithmetic types (everything
  else passes a reference to the base variant constructor)

- also removes `is_same<decay_t<T>, value>` SFINAE check -- because
  we're only passing a reference down, explicitly forcing the compiler
  to use the implicitly-defined copy/move instead is pointless
---
 include/mapnik/symbolizer_base.hpp | 13 ++--
 include/mapnik/value.hpp           | 30 +++------
 include/mapnik/value_types.hpp     | 97 +++++-------------------------
 3 files changed, 29 insertions(+), 111 deletions(-)

diff --git a/include/mapnik/symbolizer_base.hpp b/include/mapnik/symbolizer_base.hpp
index f07cf3d2a..8ece232d0 100644
--- a/include/mapnik/symbolizer_base.hpp
+++ b/include/mapnik/symbolizer_base.hpp
@@ -100,18 +100,13 @@ struct strict_value : value_base_type
 {
     strict_value() = default;
 
-    strict_value(const char* val)
+    strict_value(const char* val) noexcept(false)
         : value_base_type(std::string(val)) {}
 
-    template <typename T>
-    strict_value(T const& obj)
-        : value_base_type(typename detail::mapnik_value_type<T>::type(obj))
-    {}
-
-    template <typename T>
+    template <typename T, typename U = detail::mapnik_value_type_t<T>>
     strict_value(T && obj)
-        noexcept(std::is_nothrow_constructible<value_base_type, T && >::value)
-        : value_base_type(std::forward<T>(obj))
+        noexcept(std::is_nothrow_constructible<value_base_type, U>::value)
+        : value_base_type(U(std::forward<T>(obj)))
     {}
 };
 
diff --git a/include/mapnik/value.hpp b/include/mapnik/value.hpp
index 9ee732385..fb1e817d6 100644
--- a/include/mapnik/value.hpp
+++ b/include/mapnik/value.hpp
@@ -47,30 +47,20 @@ class MAPNIK_DECL value : public value_base
 public:
     value() = default;
 
-    // conversion from type T is done via a temporary of type U, which
-    // is determined by mapnik_value_type;
-    // enable_if< decay<T> != value > is necessary to avoid ill-formed
-    // recursion in noexcept specifier; and it also prevents using this
-    // constructor where implicitly-declared copy/move should be used
-    // (e.g. value(value&))
-    template <typename T,
-              typename U = typename std::enable_if<
-                                !detail::is_same_decay<T, value>::value,
-                                detail::mapnik_value_type_decay<T>
-                            >::type::type>
+    // Conversion from type T is done via a temporary value or reference
+    // of type U, which is determined by mapnik_value_type_t.
+    //
+    // CAVEAT: We don't check `noexcept(conversion from T to U)`.
+    //         But since the type U is either value_bool, value_integer,
+    //         value_double or T &&, this conversion SHOULD NEVER throw.
+    template <typename T, typename U = detail::mapnik_value_type_t<T>>
     value(T && val)
-        noexcept(noexcept(U(std::forward<T>(val))) &&
-                 std::is_nothrow_constructible<value_base, U && >::value)
+        noexcept(std::is_nothrow_constructible<value_base, U>::value)
         : value_base(U(std::forward<T>(val))) {}
 
-    template <typename T,
-              typename U = typename std::enable_if<
-                                !detail::is_same_decay<T, value>::value,
-                                detail::mapnik_value_type_decay<T>
-                            >::type::type>
+    template <typename T, typename U = detail::mapnik_value_type_t<T>>
     value& operator=(T && val)
-        noexcept(noexcept(U(std::forward<T>(val))) &&
-                 std::is_nothrow_assignable<value_base, U && >::value)
+        noexcept(std::is_nothrow_assignable<value_base, U>::value)
     {
         value_base::operator=(U(std::forward<T>(val)));
         return *this;
diff --git a/include/mapnik/value_types.hpp b/include/mapnik/value_types.hpp
index 235133a34..d1c0c59bd 100644
--- a/include/mapnik/value_types.hpp
+++ b/include/mapnik/value_types.hpp
@@ -152,90 +152,23 @@ inline std::istream& operator>> ( std::istream & s, value_null & )
 
 
 namespace detail {
-// to mapnik::value_type conversions traits
-template <typename T>
-struct is_value_bool
-{
-    constexpr static bool value = std::is_same<T, bool>::value;
-};
 
-template <typename T>
-struct is_value_integer
-{
-    constexpr static bool value = std::is_integral<T>::value && !std::is_same<T, bool>::value;
-};
+// Helper metafunction for mapnik::value construction and assignment.
+// Returns:
+//  value_bool      if T is bool
+//  value_integer   if T is an integral type (except bool)
+//  value_double    if T is a floating-point type
+//  T &&            otherwise
 
-template <typename T>
-struct is_value_double
-{
-    constexpr static bool value = std::is_floating_point<T>::value;
-};
-
-template <typename T>
-struct is_value_unicode_string
-{
-    constexpr static bool value = std::is_same<T, typename mapnik::value_unicode_string>::value;
-};
-
-template <typename T>
-struct is_value_string
-{
-    constexpr static bool value = std::is_same<T, typename std::string>::value;
-};
-
-template <typename T>
-struct is_value_null
-{
-    constexpr static bool value = std::is_same<T, typename mapnik::value_null>::value;
-};
-
-template <typename T, class Enable = void>
-struct mapnik_value_type
-{
-    using type = T;
-};
-
-// value_null
-template <typename T>
-struct mapnik_value_type<T, typename std::enable_if<detail::is_value_null<T>::value>::type>
-{
-    using type = mapnik::value_null;
-};
-
-// value_bool
-template <typename T>
-struct mapnik_value_type<T, typename std::enable_if<detail::is_value_bool<T>::value>::type>
-{
-    using type = mapnik::value_bool;
-};
-
-// value_integer
-template <typename T>
-struct mapnik_value_type<T, typename std::enable_if<detail::is_value_integer<T>::value>::type>
-{
-    using type = mapnik::value_integer;
-};
-
-// value_double
-template <typename T>
-struct mapnik_value_type<T, typename std::enable_if<detail::is_value_double<T>::value>::type>
-{
-    using type = mapnik::value_double;
-};
-
-// value_unicode_string
-template <typename T>
-struct mapnik_value_type<T, typename std::enable_if<detail::is_value_unicode_string<T>::value>::type>
-{
-    using type = mapnik::value_unicode_string const&;
-};
-
-template <typename T>
-using mapnik_value_type_decay = mapnik_value_type<typename std::decay<T>::type>;
-
-template <typename T, typename U>
-using is_same_decay = std::is_same<typename std::decay<T>::type,
-                                   typename std::decay<U>::type>;
+template <typename T, typename dT = std::decay_t<T>>
+using mapnik_value_type_t =
+    std::conditional_t<
+        std::is_same<dT, bool>::value, value_bool,
+        std::conditional_t<
+            std::is_integral<dT>::value, value_integer,
+            std::conditional_t<
+                std::is_floating_point<dT>::value, value_double,
+                T && >>>;
 
 } // namespace detail