diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d68eb9d..378f1914 100644 --- a/CHANGELOG.md +++ b/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) ------- diff --git a/extras/tests/JsonVariant/set.cpp b/extras/tests/JsonVariant/set.cpp index eeb9088b..0baa22d2 100644 --- a/extras/tests/JsonVariant/set.cpp +++ b/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(); - SECTION("stores const char* by reference") { + SECTION("const char*") { char str[16]; strcpy(str, "hello"); - variant.set(static_cast(str)); + bool result = variant.set(static_cast(str)); strcpy(str, "world"); - REQUIRE(variant == "world"); + REQUIRE(result == true); + REQUIRE(variant == "world"); // stores by pointer } - SECTION("stores char* by copy") { - char str[16]; + SECTION("(const char*)0") { + bool result = variant.set(static_cast(0)); - strcpy(str, "hello"); - variant.set(str); - strcpy(str, "world"); - - REQUIRE(variant == "hello"); + REQUIRE(result == true); + REQUIRE(variant.isNull()); } - SECTION("stores unsigned char* by copy") { + SECTION("char*") { char str[16]; strcpy(str, "hello"); - variant.set(reinterpret_cast(str)); + bool result = variant.set(str); strcpy(str, "world"); - REQUIRE(variant == "hello"); + REQUIRE(result == true); + REQUIRE(variant == "hello"); // stores by copy } - SECTION("stores signed char* by copy") { + SECTION("(char*)0") { + bool result = variant.set(static_cast(0)); + + REQUIRE(result == true); + REQUIRE(variant.isNull()); + } + + SECTION("unsigned char*") { char str[16]; strcpy(str, "hello"); - variant.set(reinterpret_cast(str)); + bool result = variant.set(reinterpret_cast(str)); strcpy(str, "world"); - REQUIRE(variant == "hello"); + REQUIRE(result == true); + REQUIRE(variant == "hello"); // stores by copy + } + + SECTION("signed char*") { + char str[16]; + + strcpy(str, "hello"); + bool result = variant.set(reinterpret_cast(str)); + strcpy(str, "world"); + + 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() == true); REQUIRE(variant.as() == 10); } } -TEST_CASE("JsonVariant with not enough memory") { +TEST_CASE("JsonVariant::set() with not enough memory") { StaticJsonDocument<1> doc; JsonVariant v = doc.to(); 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") { - 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()); } } diff --git a/src/ArduinoJson/Json/JsonDeserializer.hpp b/src/ArduinoJson/Json/JsonDeserializer.hpp index 9acf3b53..29e1a836 100644 --- a/src/ArduinoJson/Json/JsonDeserializer.hpp +++ b/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; } diff --git a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp b/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp index d068c871..6802798f 100644 --- a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp +++ b/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; } diff --git a/src/ArduinoJson/Polyfills/gsl/not_null.hpp b/src/ArduinoJson/Polyfills/gsl/not_null.hpp deleted file mode 100644 index 79df0a1d..00000000 --- a/src/ArduinoJson/Polyfills/gsl/not_null.hpp +++ /dev/null @@ -1,34 +0,0 @@ -// ArduinoJson - arduinojson.org -// Copyright Benoit Blanchon 2014-2020 -// MIT License - -#pragma once - -#include -#include - -namespace ARDUINOJSON_NAMESPACE { - -template -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 -not_null make_not_null(T ptr) { - ARDUINOJSON_ASSERT(ptr != NULL); - return not_null(ptr); -} - -} // namespace ARDUINOJSON_NAMESPACE diff --git a/src/ArduinoJson/Variant/VariantData.hpp b/src/ArduinoJson/Variant/VariantData.hpp index 1c10c5f4..86486a7e 100644 --- a/src/ArduinoJson/Variant/VariantData.hpp +++ b/src/ArduinoJson/Variant/VariantData.hpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -245,25 +244,14 @@ class VariantData { setType(VALUE_IS_NULL); } - void setString(not_null 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 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 - 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 @@ -283,14 +271,27 @@ class VariantData { template 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 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() { diff --git a/src/ArduinoJson/Variant/VariantSlot.hpp b/src/ArduinoJson/Variant/VariantSlot.hpp index bc5dd378..af646709 100644 --- a/src/ArduinoJson/Variant/VariantSlot.hpp +++ b/src/ArduinoJson/Variant/VariantSlot.hpp @@ -6,7 +6,6 @@ #include // int8_t, int16_t -#include #include #include #include