JsonDocument: use the copy-and-swap idiom

extras/tests/ResourceManager/CMakeLists.txt

@@ -9,6 +9,7 @@ add_executable(ResourceManagerTests
 	shrinkToFit.cpp
 	size.cpp
 	StringBuilder.cpp
+	swap.cpp
 )
 
 add_compile_definitions(ResourceManagerTests

extras/tests/ResourceManager/swap.cpp

@@ -0,0 +1,95 @@
+// ArduinoJson - https://arduinojson.org
+// Copyright © 2014-2023, Benoit BLANCHON
+// MIT License
+
+#include <ArduinoJson/Memory/Alignment.hpp>
+#include <ArduinoJson/Memory/ResourceManager.hpp>
+#include <ArduinoJson/Memory/VariantPoolImpl.hpp>
+#include <catch.hpp>
+
+#include "Allocators.hpp"
+
+using namespace ArduinoJson::detail;
+
+static void fullPreallocatedPools(ResourceManager& resources) {
+  for (int i = 0;
+       i < ARDUINOJSON_INITIAL_POOL_COUNT * ARDUINOJSON_POOL_CAPACITY; i++)
+    resources.allocSlot();
+}
+
+TEST_CASE("ResourceManager::swap()") {
+  SECTION("Both using preallocated pool list") {
+    SpyingAllocator allocator;
+    ResourceManager a(&allocator);
+    ResourceManager b(&allocator);
+
+    auto a1 = a.allocSlot();
+    auto b1 = b.allocSlot();
+
+    swap(a, b);
+
+    REQUIRE(a1->data() == b.getSlot(a1.id())->data());
+    REQUIRE(b1->data() == a.getSlot(b1.id())->data());
+
+    REQUIRE(allocator.log() == AllocatorLog()
+                                   << AllocatorLog::Allocate(sizeofPool()) * 2);
+  }
+
+  SECTION("Only left using preallocated pool list") {
+    SpyingAllocator allocator;
+    ResourceManager a(&allocator);
+    ResourceManager b(&allocator);
+    fullPreallocatedPools(b);
+
+    auto a1 = a.allocSlot();
+    auto b1 = b.allocSlot();
+    swap(a, b);
+
+    REQUIRE(a1->data() == b.getSlot(a1.id())->data());
+    REQUIRE(b1->data() == a.getSlot(b1.id())->data());
+
+    REQUIRE(allocator.log() == AllocatorLog()
+                                   << AllocatorLog::Allocate(sizeofPool()) *
+                                          (ARDUINOJSON_INITIAL_POOL_COUNT + 1)
+                                   << AllocatorLog::Allocate(sizeofPoolList(
+                                          ARDUINOJSON_INITIAL_POOL_COUNT * 2))
+                                   << AllocatorLog::Allocate(sizeofPool()));
+  }
+
+  SECTION("Only right using preallocated pool list") {
+    SpyingAllocator allocator;
+    ResourceManager a(&allocator);
+    fullPreallocatedPools(a);
+    ResourceManager b(&allocator);
+
+    auto a1 = a.allocSlot();
+    auto b1 = b.allocSlot();
+    swap(a, b);
+
+    REQUIRE(a1->data() == b.getSlot(a1.id())->data());
+    REQUIRE(b1->data() == a.getSlot(b1.id())->data());
+
+    REQUIRE(allocator.log() == AllocatorLog()
+                                   << AllocatorLog::Allocate(sizeofPool()) *
+                                          ARDUINOJSON_INITIAL_POOL_COUNT
+                                   << AllocatorLog::Allocate(sizeofPoolList(
+                                          ARDUINOJSON_INITIAL_POOL_COUNT * 2))
+                                   << AllocatorLog::Allocate(sizeofPool()) * 2);
+  }
+
+  SECTION("None is using preallocated pool list") {
+    SpyingAllocator allocator;
+    ResourceManager a(&allocator);
+    fullPreallocatedPools(a);
+    ResourceManager b(&allocator);
+    fullPreallocatedPools(b);
+
+    auto a1 = a.allocSlot();
+    auto b1 = b.allocSlot();
+
+    swap(a, b);
+
+    REQUIRE(a1->data() == b.getSlot(a1.id())->data());
+    REQUIRE(b1->data() == a.getSlot(b1.id())->data());
+  }
+}

src/ArduinoJson/Document/JsonDocument.hpp

@@ -30,9 +30,8 @@ class JsonDocument : public detail::VariantOperators<const JsonDocument&> {
   }
 
   // Move-constructor
-  JsonDocument(JsonDocument&& src) : resources_(src.allocator()) {
-    // TODO: use the copy and swap idiom
-    moveAssignFrom(src);
+  JsonDocument(JsonDocument&& src) : JsonDocument() {
+    swap(*this, src);
   }
 
   // Construct from variant, array, or object
@@ -56,15 +55,8 @@ class JsonDocument : public detail::VariantOperators<const JsonDocument&> {
     set(src);
   }
 
-  JsonDocument& operator=(const JsonDocument& src) {
-    // TODO: use the copy and swap idiom
-    copyAssignFrom(src);
-    return *this;
-  }
-
-  JsonDocument& operator=(JsonDocument&& src) {
-    // TODO: use the copy and swap idiom
-    moveAssignFrom(src);
+  JsonDocument& operator=(JsonDocument src) {
+    swap(*this, src);
     return *this;
   }
 
@@ -87,11 +79,11 @@ class JsonDocument : public detail::VariantOperators<const JsonDocument&> {
   // Reclaims the memory leaked when removing and replacing values.
   // https://arduinojson.org/v6/api/jsondocument/garbagecollect/
   bool garbageCollect() {
-    // make a temporary clone and move assign
+    // make a temporary clone and swap
     JsonDocument tmp(*this);
     if (tmp.overflowed())
       return false;
-    moveAssignFrom(tmp);
+    swap(*this, tmp);
     return true;
   }
 
@@ -346,6 +338,11 @@ class JsonDocument : public detail::VariantOperators<const JsonDocument&> {
     return getSlot();
   }
 
+  friend void swap(JsonDocument& a, JsonDocument& b) {
+    swap(a.resources_, b.resources_);
+    swap_(a.data_, b.data_);
+  }
+
  private:
   JsonVariant getSlot() {
     return JsonVariant(&data_, &resources_);
@@ -355,16 +352,6 @@ class JsonDocument : public detail::VariantOperators<const JsonDocument&> {
     return JsonVariantConst(&data_, &resources_);
   }
 
-  void copyAssignFrom(const JsonDocument& src) {
-    set(src);
-  }
-
-  void moveAssignFrom(JsonDocument& src) {
-    data_ = src.data_;
-    src.data_.reset();
-    resources_ = move(src.resources_);
-  }
-
   detail::ResourceManager* getResourceManager() {
     return &resources_;
   }

src/ArduinoJson/Memory/ResourceManager.hpp

@@ -29,14 +29,11 @@ class ResourceManager {
   ResourceManager(const ResourceManager&) = delete;
   ResourceManager& operator=(const ResourceManager& src) = delete;
 
-  ResourceManager& operator=(ResourceManager&& src) {
-    stringPool_.clear(allocator_);
-    variantPools_.clear(allocator_);
-    allocator_ = src.allocator_;
-    variantPools_ = detail::move(src.variantPools_);
-    overflowed_ = src.overflowed_;
-    stringPool_ = detail::move(src.stringPool_);
-    return *this;
+  friend void swap(ResourceManager& a, ResourceManager& b) {
+    swap(a.stringPool_, b.stringPool_);
+    swap(a.variantPools_, b.variantPools_);
+    swap_(a.allocator_, b.allocator_);
+    swap_(a.overflowed_, b.overflowed_);
   }
 
   Allocator* allocator() const {

src/ArduinoJson/Memory/StringPool.hpp

@@ -19,15 +19,14 @@ class StringPool {
  public:
   StringPool() = default;
   StringPool(const StringPool&) = delete;
+  void operator=(StringPool&& src) = delete;
 
   ~StringPool() {
     ARDUINOJSON_ASSERT(strings_ == nullptr);
   }
 
-  void operator=(StringPool&& src) {
-    ARDUINOJSON_ASSERT(strings_ == nullptr);
-    strings_ = src.strings_;
-    src.strings_ = nullptr;
+  friend void swap(StringPool& a, StringPool& b) {
+    swap_(a.strings_, b.strings_);
   }
 
   void clear(Allocator* allocator) {

src/ArduinoJson/Memory/VariantPoolList.hpp

@@ -19,6 +19,37 @@ class VariantPoolList {
     ARDUINOJSON_ASSERT(count_ == 0);
   }
 
+  friend void swap(VariantPoolList& a, VariantPoolList& b) {
+    bool aUsedPreallocated = a.pools_ == a.preallocatedPools_;
+    bool bUsedPreallocated = b.pools_ == b.preallocatedPools_;
+
+    // Who is using preallocated pools?
+    if (aUsedPreallocated && bUsedPreallocated) {
+      // both of us => swap preallocated pools
+      for (PoolCount i = 0; i < ARDUINOJSON_INITIAL_POOL_COUNT; i++)
+        swap_(a.preallocatedPools_[i], b.preallocatedPools_[i]);
+    } else if (bUsedPreallocated) {
+      // only b => copy b's preallocated pools and give him a's pointer
+      for (PoolCount i = 0; i < b.count_; i++)
+        a.preallocatedPools_[i] = b.preallocatedPools_[i];
+      b.pools_ = a.pools_;
+      a.pools_ = a.preallocatedPools_;
+    } else if (aUsedPreallocated) {
+      // only a => copy a's preallocated pools and give him b's pointer
+      for (PoolCount i = 0; i < a.count_; i++)
+        b.preallocatedPools_[i] = a.preallocatedPools_[i];
+      a.pools_ = b.pools_;
+      b.pools_ = b.preallocatedPools_;
+    } else {
+      // neither => swap pointers
+      swap_(a.pools_, b.pools_);
+    }
+
+    swap_(a.count_, b.count_);
+    swap_(a.capacity_, b.capacity_);
+    swap_(a.freeList_, b.freeList_);
+  }
+
   VariantPoolList& operator=(VariantPoolList&& src) {
     ARDUINOJSON_ASSERT(count_ == 0);
     if (src.pools_ == src.preallocatedPools_) {

src/ArduinoJson/Polyfills/utility.hpp

@@ -20,4 +20,14 @@ typename remove_reference<T>::type&& move(T&& t) {
   return static_cast<typename remove_reference<T>::type&&>(t);
 }
 
+// Polyfull for std::swap
+// Don't use the name "swap" because it makes calls ambiguous for types in the
+// detail namespace
+template <class T>
+void swap_(T& a, T& b) {
+  T tmp = move(a);
+  a = move(b);
+  b = move(tmp);
+}
+
 ARDUINOJSON_END_PRIVATE_NAMESPACE

src/ArduinoJson/Variant/VariantData.hpp

@@ -302,11 +302,6 @@ class VariantData {
     return var->nesting(resources);
   }
 
-  void operator=(const VariantData& src) {
-    content_ = src.content_;
-    flags_ = uint8_t((flags_ & OWNED_KEY_BIT) | (src.flags_ & ~OWNED_KEY_BIT));
-  }
-
   void removeElement(size_t index, ResourceManager* resources) {
     ArrayData::removeElement(asArray(), index, resources);
   }