Add default constructor to dynamic, make other constructors simpler and stricter

Summary:
Now that the initializer list syntax has been removed we can add a default constructor.
Also,
- The `dynamic(T)` constructor was unconstrained, so it would match any type but then fail to compile (as a side effect, `is_convertible<T, dynamic>` would be always true). This also leaked the implementation details of `Array` and `Object`, as they were accepted as arguments. The diff makes the constructor accept only integral and float arguments, and all other types are SFINAEd out.
- `dynamic(Iterator, Iterator)` is made `explicit` to avoid accepting statements like `dynamic d = {"a", "b"};`.
- `object(...)` methods are simplified.

Reviewed By: luciang

Differential Revision: D4065021

fbshipit-source-id: ac289da7bece67c674b7036b7b51d5e016b297e5

folly/docs/Dynamic.md

@@ -25,7 +25,7 @@ folly::dynamic;` was used):
     dynamic nul = nullptr;
     dynamic boolean = false;
 
-    // Arrays can be initialized with brackets.
+    // Arrays can be initialized with dynamic::array.
     dynamic array = dynamic::array("array ", "of ", 4, " elements");
     assert(array.size() == 4);
     dynamic emptyArray = dynamic::array;
@@ -173,19 +173,6 @@ static types, etc).
 ### Some Design Rationale
 ***
 
-**Q. Why is there no default constructor?**
-
-This is a bit of a limitation of `std::initializer_list<>` for
-this use case. The expression `dynamic d = {}` is required by the
-standard to call the default constructor if one exists (the
-reasoning for this makes sense, since `{}` is part of the concept
-of "uniform initialization", and is intended for use with things
-like `std::vector`). It would be surprising if this expression
-didn't leave `d.isArray()` true, but on the other hand it would
-also be surprising if `dynamic d` left `d.isArray()` as true. The
-solution was just to disallow uninitialized dynamics: every
-dynamic must start out being assigned to some value (or nullptr).
-
 **Q. Why doesn't a dynamic string support begin(), end(), and operator[]?**
 
 The value_type of a dynamic iterator is `dynamic`, and `operator[]`

folly/dynamic-inl.h

@@ -92,44 +92,6 @@ namespace detail {
     template<class T> static void destroy(T* t) { t->~T(); }
   };
 
-  /*
-   * The enable_if junk here is necessary to avoid ambiguous
-   * conversions relating to bool and double when you implicitly
-   * convert an int or long to a dynamic.
-   */
-  template<class T, class Enable = void> struct ConversionHelper;
-  template<class T>
-  struct ConversionHelper<
-    T,
-    typename std::enable_if<
-      std::is_integral<T>::value && !std::is_same<T,bool>::value
-    >::type
-  > {
-    typedef int64_t type;
-  };
-  template <>
-  struct ConversionHelper<float> {
-    typedef double type;
-  };
-  template <class T>
-  struct ConversionHelper<
-      T,
-      typename std::enable_if<
-          (!std::is_integral<T>::value || std::is_same<T, bool>::value) &&
-          !std::is_same<T, float>::value &&
-          !std::is_same<T, std::nullptr_t>::value>::type> {
-    typedef T type;
-  };
-  template<class T>
-  struct ConversionHelper<
-    T,
-    typename std::enable_if<
-      std::is_same<T,std::nullptr_t>::value
-    >::type
-  > {
-    typedef void* type;
-  };
-
   /*
    * Helper for implementing numeric conversions in operators on
    * numbers.  Just promotes to double when one of the arguments is
@@ -173,12 +135,7 @@ struct dynamic::ObjectMaker {
   friend struct dynamic;
 
   explicit ObjectMaker() : val_(dynamic::object) {}
-  explicit ObjectMaker(dynamic const& key, dynamic val)
-    : val_(dynamic::object)
-  {
-    val_.insert(key, std::move(val));
-  }
-  explicit ObjectMaker(dynamic&& key, dynamic val)
+  explicit ObjectMaker(dynamic key, dynamic val)
     : val_(dynamic::object)
   {
     val_.insert(std::move(key), std::move(val));
@@ -191,15 +148,10 @@ struct dynamic::ObjectMaker {
   ObjectMaker& operator=(ObjectMaker const&) = delete;
   ObjectMaker& operator=(ObjectMaker&&) = delete;
 
-  // These return rvalue-references instead of lvalue-references to allow
-  // constructs like this to moved instead of copied:
+  // This returns an rvalue-reference instead of an lvalue-reference
+  // to allow constructs like this to moved instead of copied:
   //  dynamic a = dynamic::object("a", "b")("c", "d")
-  ObjectMaker&& operator()(dynamic const& key, dynamic val) {
-    val_.insert(key, std::move(val));
-    return std::move(*this);
-  }
-
-  ObjectMaker&& operator()(dynamic&& key, dynamic val) {
+  ObjectMaker&& operator()(dynamic key, dynamic val) {
     val_.insert(std::move(key), std::move(val));
     return std::move(*this);
   }
@@ -215,26 +167,10 @@ inline dynamic dynamic::array(Args&& ...args) {
   return dynamic(Array{std::forward<Args>(args)...});
 }
 
-// This looks like a case for perfect forwarding, but our use of
-// std::initializer_list for constructing dynamic arrays makes it less
-// functional than doing this manually.
-
-// TODO(ott, 10300209): When the initializer_list constructor is gone,
-// simplify this.
 inline dynamic::ObjectMaker dynamic::object() { return ObjectMaker(); }
-inline dynamic::ObjectMaker dynamic::object(dynamic&& a, dynamic&& b) {
+inline dynamic::ObjectMaker dynamic::object(dynamic a, dynamic b) {
   return ObjectMaker(std::move(a), std::move(b));
 }
-inline dynamic::ObjectMaker dynamic::object(dynamic const& a, dynamic&& b) {
-  return ObjectMaker(a, std::move(b));
-}
-inline dynamic::ObjectMaker dynamic::object(dynamic&& a, dynamic const& b) {
-  return ObjectMaker(std::move(a), b);
-}
-inline dynamic::ObjectMaker
-dynamic::object(dynamic const& a, dynamic const& b) {
-  return ObjectMaker(a, b);
-}
 
 //////////////////////////////////////////////////////////////////////
 
@@ -275,6 +211,10 @@ struct dynamic::const_value_iterator
 
 //////////////////////////////////////////////////////////////////////
 
+inline dynamic::dynamic() : dynamic(nullptr) {}
+
+inline dynamic::dynamic(std::nullptr_t) : type_(NULLT) {}
+
 inline dynamic::dynamic(void (*)(EmptyArrayTag))
   : type_(ARRAY)
 {
@@ -299,13 +239,9 @@ inline dynamic::dynamic(char const* s)
   new (&u_.string) std::string(s);
 }
 
-inline dynamic::dynamic(std::string const& s)
+inline dynamic::dynamic(std::string s)
   : type_(STRING)
 {
-  new (&u_.string) std::string(s);
-}
-
-inline dynamic::dynamic(std::string&& s) : type_(STRING) {
   new (&u_.string) std::string(std::move(s));
 }
 
@@ -330,14 +266,32 @@ inline dynamic::dynamic(dynamic&& o) noexcept
 
 inline dynamic::~dynamic() noexcept { destroy(); }
 
-template<class T>
+// Integral types except bool convert to int64_t, float types to double.
+template <class T>
+struct dynamic::NumericTypeHelper<
+    T, typename std::enable_if<std::is_integral<T>::value>::type> {
+  using type = int64_t;
+};
+template <>
+struct dynamic::NumericTypeHelper<bool> {
+  using type = bool;
+};
+template <>
+struct dynamic::NumericTypeHelper<float> {
+  using type = double;
+};
+template <>
+struct dynamic::NumericTypeHelper<double> {
+  using type = double;
+};
+
+template<class T, class NumericType /* = typename NumericTypeHelper<T>::type */>
 dynamic::dynamic(T t) {
-  typedef typename detail::ConversionHelper<T>::type U;
-  type_ = TypeInfo<U>::type;
-  new (getAddress<U>()) U(std::move(t));
+  type_ = TypeInfo<NumericType>::type;
+  new (getAddress<NumericType>()) NumericType(t);
 }
 
-template<class Iterator>
+template <class Iterator>
 dynamic::dynamic(Iterator first, Iterator last)
   : type_(ARRAY)
 {

folly/dynamic.h

@@ -45,16 +45,6 @@
  * Also see folly/json.h for the serialization and deserialization
  * functions for JSON.
  *
- * Note: dynamic is not DefaultConstructible.  Rationale:
- *
- *   - The intuitive thing to initialize a defaulted dynamic to would
- *     be nullptr.
- *
- *   - However, the expression dynamic d = {} is required to call the
- *     default constructor by the standard, which is confusing
- *     behavior for dynamic unless the default constructor creates an
- *     empty array.
- *
  * Additional documentation is in folly/docs/Dynamic.md.
  *
  * @author Jordan DeLong <delong.j@fb.com>
@@ -95,6 +85,7 @@ struct dynamic : private boost::operators<dynamic> {
     OBJECT,
     STRING,
   };
+  template<class T, class Enable = void> struct NumericTypeHelper;
 
   /*
    * We support direct iteration of arrays, and indirect iteration of objects.
@@ -143,18 +134,20 @@ public:
   static dynamic array(Args&& ...args);
 
   static ObjectMaker object();
-  static ObjectMaker object(dynamic&&, dynamic&&);
-  static ObjectMaker object(dynamic const&, dynamic&&);
-  static ObjectMaker object(dynamic&&, dynamic const&);
-  static ObjectMaker object(dynamic const&, dynamic const&);
+  static ObjectMaker object(dynamic, dynamic);
+
+  /**
+   * Default constructor, initializes with nullptr.
+   */
+  dynamic();
 
   /*
    * String compatibility constructors.
    */
+  /* implicit */ dynamic(std::nullptr_t);
   /* implicit */ dynamic(StringPiece val);
   /* implicit */ dynamic(char const* val);
-  /* implicit */ dynamic(std::string const& val);
-  /* implicit */ dynamic(std::string&& val);
+  /* implicit */ dynamic(std::string val);
 
   /*
    * This is part of the plumbing for array() and object(), above.
@@ -166,15 +159,18 @@ public:
   /* implicit */ dynamic(ObjectMaker&&);
 
   /*
-   * Conversion constructors from most of the other types.
+   * Constructors for integral and float types.
+   * Other types are SFINAEd out with NumericTypeHelper.
    */
-  template<class T> /* implicit */ dynamic(T t);
+  template<class T, class NumericType = typename NumericTypeHelper<T>::type>
+  /* implicit */ dynamic(T t);
 
   /*
    * Create a dynamic that is an array of the values from the supplied
    * iterator range.
    */
-  template<class Iterator> dynamic(Iterator first, Iterator last);
+  template<class Iterator>
+  explicit dynamic(Iterator first, Iterator last);
 
   dynamic(dynamic const&);
   dynamic(dynamic&&) noexcept;

folly/test/DynamicTest.cpp

@@ -31,6 +31,11 @@ void dynamic::print_as_pseudo_json(std::ostream& out) const {
   out << "<folly::dynamic object of type " << type_ << ">";
 }
 
+TEST(Dynamic, Default) {
+  dynamic obj;
+  EXPECT_TRUE(obj.isNull());
+}
+
 TEST(Dynamic, ObjectBasics) {
   dynamic obj = dynamic::object("a", false);
   EXPECT_EQ(obj.at("a"), false);