Fixed result of `JsonVariant::set((char*)0)` (fixes #1368)

CHANGELOG.md

@@ -6,6 +6,7 @@ HEAD
 
 * Added a build failure when nullptr is defined as a macro (issue #1355)
 * Added `JsonDocument::overflowed()` which tells if the memory pool was too small (issue #1358)
+* Fixed `JsonVariant::set((char*)0)` which returned false instead of true (issue #1368)
 
 v6.16.1 (2020-08-04)
 -------

extras/tests/JsonVariant/set.cpp

@@ -7,115 +7,150 @@
 
 enum ErrorCode { ERROR_01 = 1, ERROR_10 = 10 };
 
-TEST_CASE("JsonVariant and strings") {
+TEST_CASE("JsonVariant::set() when there is enough memory") {
   DynamicJsonDocument doc(4096);
   JsonVariant variant = doc.to<JsonVariant>();
 
-  SECTION("stores const char* by reference") {
+  SECTION("const char*") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(static_cast<const char *>(str));
+    bool result = variant.set(static_cast<const char *>(str));
     strcpy(str, "world");
 
-    REQUIRE(variant == "world");
+    REQUIRE(result == true);
+    REQUIRE(variant == "world");  // stores by pointer
   }
 
-  SECTION("stores char* by copy") {
+  SECTION("(const char*)0") {
+    bool result = variant.set(static_cast<const char *>(0));
+
+    REQUIRE(result == true);
+    REQUIRE(variant.isNull());
+  }
+
+  SECTION("char*") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(str);
+    bool result = variant.set(str);
     strcpy(str, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 
-  SECTION("stores unsigned char* by copy") {
+  SECTION("(char*)0") {
+    bool result = variant.set(static_cast<char *>(0));
+
+    REQUIRE(result == true);
+    REQUIRE(variant.isNull());
+  }
+
+  SECTION("unsigned char*") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(reinterpret_cast<unsigned char *>(str));
+    bool result = variant.set(reinterpret_cast<unsigned char *>(str));
     strcpy(str, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 
-  SECTION("stores signed char* by copy") {
+  SECTION("signed char*") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(reinterpret_cast<signed char *>(str));
+    bool result = variant.set(reinterpret_cast<signed char *>(str));
     strcpy(str, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 
 #ifdef HAS_VARIABLE_LENGTH_ARRAY
-  SECTION("stores VLA by copy") {
+  SECTION("VLA") {
     int n = 16;
     char str[n];
 
     strcpy(str, "hello");
-    variant.set(str);
+    bool result = variant.set(str);
     strcpy(str, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 #endif
 
-  SECTION("stores std::string by copy") {
+  SECTION("std::string") {
     std::string str;
 
     str = "hello";
-    variant.set(str);
+    bool result = variant.set(str);
     str.replace(0, 5, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 
-  SECTION("stores static JsonString by reference") {
+  SECTION("static JsonString") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(JsonString(str, true));
+    bool result = variant.set(JsonString(str, true));
     strcpy(str, "world");
 
-    REQUIRE(variant == "world");
+    REQUIRE(result == true);
+    REQUIRE(variant == "world");  // stores by pointer
   }
 
-  SECTION("stores non-static JsonString by copy") {
+  SECTION("non-static JsonString") {
     char str[16];
 
     strcpy(str, "hello");
-    variant.set(JsonString(str, false));
+    bool result = variant.set(JsonString(str, false));
     strcpy(str, "world");
 
-    REQUIRE(variant == "hello");
+    REQUIRE(result == true);
+    REQUIRE(variant == "hello");  // stores by copy
   }
 
-  SECTION("stores an enum as an integer") {
+  SECTION("enum") {
     ErrorCode code = ERROR_10;
 
-    variant.set(code);
+    bool result = variant.set(code);
 
+    REQUIRE(result == true);
     REQUIRE(variant.is<int>() == true);
     REQUIRE(variant.as<int>() == 10);
   }
 }
 
-TEST_CASE("JsonVariant with not enough memory") {
+TEST_CASE("JsonVariant::set() with not enough memory") {
   StaticJsonDocument<1> doc;
 
   JsonVariant v = doc.to<JsonVariant>();
 
   SECTION("std::string") {
-    v.set(std::string("hello world!!"));
+    bool result = v.set(std::string("hello world!!"));
+
+    REQUIRE(result == false);
     REQUIRE(v.isNull());
   }
 
   SECTION("Serialized<std::string>") {
-    v.set(serialized(std::string("hello world!!")));
+    bool result = v.set(serialized(std::string("hello world!!")));
+
+    REQUIRE(result == false);
+    REQUIRE(v.isNull());
+  }
+
+  SECTION("char*") {
+    char s[] = "hello world!!";
+    bool result = v.set(s);
+
+    REQUIRE(result == false);
     REQUIRE(v.isNull());
   }
 }

src/ArduinoJson/Json/JsonDeserializer.hpp

@@ -347,8 +347,7 @@ class JsonDeserializer {
     if (!parseQuotedString())
       return false;
     const char *value = _stringStorage.save();
-    variant.setString(make_not_null(value),
-                      typename TStringStorage::storage_policy());
+    variant.setStringPointer(value, typename TStringStorage::storage_policy());
     return true;
   }
 

src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp

@@ -233,8 +233,7 @@ class MsgPackDeserializer {
     const char *s = 0;  // <- mute "maybe-uninitialized" (+4 bytes on AVR)
     if (!readString(s, n))
       return false;
-    variant.setString(make_not_null(s),
-                      typename TStringStorage::storage_policy());
+    variant.setStringPointer(s, typename TStringStorage::storage_policy());
     return true;
   }
 

src/ArduinoJson/Polyfills/gsl/not_null.hpp

@@ -1,34 +0,0 @@
-// ArduinoJson - arduinojson.org
-// Copyright Benoit Blanchon 2014-2020
-// MIT License
-
-#pragma once
-
-#include <ArduinoJson/Namespace.hpp>
-#include <ArduinoJson/Polyfills/assert.hpp>
-
-namespace ARDUINOJSON_NAMESPACE {
-
-template <typename T>
-class not_null {
- public:
-  explicit not_null(T ptr) : _ptr(ptr) {
-    ARDUINOJSON_ASSERT(ptr != NULL);
-  }
-
-  T get() const {
-    ARDUINOJSON_ASSERT(_ptr != NULL);
-    return _ptr;
-  }
-
- private:
-  T _ptr;
-};
-
-template <typename T>
-not_null<T> make_not_null(T ptr) {
-  ARDUINOJSON_ASSERT(ptr != NULL);
-  return not_null<T>(ptr);
-}
-
-}  // namespace ARDUINOJSON_NAMESPACE

src/ArduinoJson/Variant/VariantData.hpp

@@ -7,7 +7,6 @@
 #include <ArduinoJson/Memory/MemoryPool.hpp>
 #include <ArduinoJson/Misc/SerializedValue.hpp>
 #include <ArduinoJson/Numbers/convertNumber.hpp>
-#include <ArduinoJson/Polyfills/gsl/not_null.hpp>
 #include <ArduinoJson/Strings/RamStringAdapter.hpp>
 #include <ArduinoJson/Variant/VariantContent.hpp>
 
@@ -245,25 +244,14 @@ class VariantData {
     setType(VALUE_IS_NULL);
   }
 
-  void setString(not_null<const char *> s, storage_policies::store_by_copy) {
+  void setStringPointer(const char *s, storage_policies::store_by_copy) {
     setType(VALUE_IS_OWNED_STRING);
-    _content.asString = s.get();
+    _content.asString = s;
   }
 
-  void setString(not_null<const char *> s, storage_policies::store_by_address) {
+  void setStringPointer(const char *s, storage_policies::store_by_address) {
     setType(VALUE_IS_LINKED_STRING);
-    _content.asString = s.get();
-  }
-
-  template <typename TStoragePolicy>
-  bool setString(const char *s, TStoragePolicy storage_policy) {
-    if (s) {
-      setString(make_not_null(s), storage_policy);
-      return true;
-    } else {
-      setType(VALUE_IS_NULL);
-      return false;
-    }
+    _content.asString = s;
   }
 
   template <typename TAdaptedString>
@@ -283,14 +271,27 @@ class VariantData {
   template <typename TAdaptedString>
   inline bool setString(TAdaptedString value, MemoryPool *,
                         storage_policies::store_by_address) {
-    return setString(value.data(), storage_policies::store_by_address());
+    if (value.isNull())
+      setNull();
+    else
+      setStringPointer(value.data(), storage_policies::store_by_address());
+    return true;
   }
 
   template <typename TAdaptedString>
   inline bool setString(TAdaptedString value, MemoryPool *pool,
                         storage_policies::store_by_copy) {
-    return setString(pool->saveString(value),
-                     storage_policies::store_by_copy());
+    if (value.isNull()) {
+      setNull();
+      return true;
+    }
+    const char *copy = pool->saveString(value);
+    if (!copy) {
+      setNull();
+      return false;
+    }
+    setStringPointer(copy, storage_policies::store_by_copy());
+    return true;
   }
 
   CollectionData &toArray() {

src/ArduinoJson/Variant/VariantSlot.hpp

@@ -6,7 +6,6 @@
 
 #include <stdint.h>  // int8_t, int16_t
 
-#include <ArduinoJson/Polyfills/gsl/not_null.hpp>
 #include <ArduinoJson/Polyfills/type_traits.hpp>
 #include <ArduinoJson/Strings/StoragePolicy.hpp>
 #include <ArduinoJson/Variant/VariantContent.hpp>