Use std::vector to manage MapBuffer storage

Summary:
Replaces raw array pointer inside `MapBuffer` with `std::vector`, which is more conventional "safer" way of storing dynamically sized arrays.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D33512115

fbshipit-source-id: d875a2240f7938fd35c4c3ae0e7e91dc7456ff32

Author: Andrei Shikov

ReactCommon/react/renderer/mapbuffer/MapBuffer.cpp

@@ -14,30 +14,24 @@ namespace react {
 
 // TODO T83483191: Extend MapBuffer C++ implementation to support basic random
 // access
-MapBuffer::MapBuffer(uint8_t *const data, int32_t dataSize) {
-  react_native_assert(
-      (data != nullptr) && "Error trying to build an invalid MapBuffer");
-
-  // Should we move the memory here or document it?
-  data_ = data;
-
+MapBuffer::MapBuffer(std::vector<uint8_t> data) : bytes_(std::move(data)) {
   count_ = 0;
   memcpy(
       reinterpret_cast<uint8_t *>(&count_),
-      reinterpret_cast<const uint8_t *>(data_ + HEADER_COUNT_OFFSET),
+      bytes_.data() + HEADER_COUNT_OFFSET,
       UINT16_SIZE);
 
   // TODO T83483191: extract memcpy calls into an inline function to simplify
   // the code
-  dataSize_ = 0;
+  int32_t dataSize;
   memcpy(
-      reinterpret_cast<uint8_t *>(&dataSize_),
-      reinterpret_cast<const uint8_t *>(data_ + HEADER_BUFFER_SIZE_OFFSET),
+      reinterpret_cast<uint8_t *>(&dataSize),
+      bytes_.data() + HEADER_BUFFER_SIZE_OFFSET,
       INT_SIZE);
 
-  if (dataSize != dataSize_) {
+  if (dataSize != bytes_.size()) {
     LOG(ERROR) << "Error: Data size does not match, expected " << dataSize
-               << " found: " << dataSize_;
+               << " found: " << bytes_.size();
     abort();
   }
 }
@@ -46,7 +40,7 @@ int32_t MapBuffer::getInt(Key key) const {
   int32_t value = 0;
   memcpy(
       reinterpret_cast<uint8_t *>(&value),
-      reinterpret_cast<const uint8_t *>(data_ + getValueOffset(key)),
+      bytes_.data() + getValueOffset(key),
       INT_SIZE);
   return value;
 }
@@ -61,7 +55,7 @@ double MapBuffer::getDouble(Key key) const {
   double value = 0;
   memcpy(
       reinterpret_cast<uint8_t *>(&value),
-      reinterpret_cast<const uint8_t *>(data_ + getValueOffset(key)),
+      bytes_.data() + getValueOffset(key),
       DOUBLE_SIZE);
   return value;
 }
@@ -80,15 +74,14 @@ std::string MapBuffer::getString(Key key) const {
   int32_t offset = getInt(key);
   memcpy(
       reinterpret_cast<uint8_t *>(&stringLength),
-      reinterpret_cast<const uint8_t *>(data_ + dynamicDataOffset + offset),
+      bytes_.data() + dynamicDataOffset + offset,
       INT_SIZE);
 
   char *value = new char[stringLength];
 
   memcpy(
       reinterpret_cast<char *>(value),
-      reinterpret_cast<const char *>(
-          data_ + dynamicDataOffset + offset + INT_SIZE),
+      bytes_.data() + dynamicDataOffset + offset + INT_SIZE,
       stringLength);
 
   return std::string(value, 0, stringLength);
@@ -103,47 +96,41 @@ MapBuffer MapBuffer::getMapBuffer(Key key) const {
   int32_t offset = getInt(key);
   memcpy(
       reinterpret_cast<uint8_t *>(&mapBufferLength),
-      reinterpret_cast<const uint8_t *>(data_ + dynamicDataOffset + offset),
+      bytes_.data() + dynamicDataOffset + offset,
       INT_SIZE);
 
-  uint8_t *value = new Byte[mapBufferLength];
+  std::vector<uint8_t> value(mapBufferLength);
 
   memcpy(
-      reinterpret_cast<uint8_t *>(value),
-      reinterpret_cast<const uint8_t *>(
-          data_ + dynamicDataOffset + offset + INT_SIZE),
+      value.data(),
+      bytes_.data() + dynamicDataOffset + offset + INT_SIZE,
       mapBufferLength);
 
-  return MapBuffer(value, mapBufferLength);
+  return MapBuffer(std::move(value));
 }
 
 bool MapBuffer::isNull(Key key) const {
   return getInt(key) == NULL_VALUE;
 }
 
 int32_t MapBuffer::getBufferSize() const {
-  return dataSize_;
+  return bytes_.size();
 }
 
 void MapBuffer::copy(uint8_t *output) const {
-  memcpy(output, data_, dataSize_);
+  memcpy(output, bytes_.data(), bytes_.size());
 }
 
 uint16_t MapBuffer::getCount() const {
   uint16_t size = 0;
 
   memcpy(
-      reinterpret_cast<uint16_t *>(&size),
-      reinterpret_cast<const uint16_t *>(data_ + HEADER_COUNT_OFFSET),
-
+      reinterpret_cast<uint8_t *>(&size),
+      bytes_.data() + HEADER_COUNT_OFFSET,
       UINT16_SIZE);
 
   return size;
 }
 
-MapBuffer::~MapBuffer() {
-  delete[] data_;
-}
-
 } // namespace react
 } // namespace facebook

ReactCommon/react/renderer/mapbuffer/MapBuffer.h

@@ -10,7 +10,6 @@
 #include <react/debug/react_native_assert.h>
 #include <react/renderer/mapbuffer/primitives.h>
 
-#include <stdlib.h>
 #include <limits>
 
 namespace facebook {
@@ -34,10 +33,7 @@ namespace react {
 class MapBuffer {
  private:
   // Buffer and its size
-  const uint8_t *data_ = nullptr;
-
-  // amount of bytes in the MapBuffer
-  int32_t dataSize_ = 0;
+  std::vector<uint8_t> const bytes_;
 
   // amount of items in the MapBuffer
   uint16_t count_ = 0;
@@ -46,9 +42,13 @@ class MapBuffer {
   int32_t getDynamicDataOffset() const;
 
  public:
-  MapBuffer(uint8_t *const data, int32_t dataSize);
+  explicit MapBuffer(std::vector<uint8_t> data);
+
+  MapBuffer(MapBuffer const &buffer) = delete;
+
+  MapBuffer &operator=(MapBuffer other) = delete;
 
-  ~MapBuffer();
+  MapBuffer(MapBuffer &&buffer) = default;
 
   int32_t getInt(Key key) const;
 

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.cpp

@@ -17,8 +17,7 @@ MapBufferBuilder::MapBufferBuilder()
     : MapBufferBuilder::MapBufferBuilder(INITIAL_KEY_VALUE_SIZE) {}
 
 MapBuffer MapBufferBuilder::EMPTY() {
-  static auto emptyMap = MapBufferBuilder().build();
-  return emptyMap;
+  return MapBufferBuilder().build();
 }
 
 MapBufferBuilder::MapBufferBuilder(uint16_t initialSize) {
@@ -181,16 +180,18 @@ MapBuffer MapBufferBuilder::build() {
   // Copy header at the beginning of "keyValues_"
   memcpy(keyValues_, &_header, HEADER_SIZE);
 
-  uint8_t *buffer = new Byte[bufferSize];
+  std::vector<uint8_t> buffer(bufferSize);
 
-  memcpy(buffer, keyValues_, keyValuesOffset_);
+  memcpy(buffer.data(), keyValues_, keyValuesOffset_);
 
   if (dynamicDataValues_ != nullptr) {
-    memcpy(buffer + keyValuesOffset_, dynamicDataValues_, dynamicDataOffset_);
+    memcpy(
+        buffer.data() + keyValuesOffset_,
+        dynamicDataValues_,
+        dynamicDataOffset_);
   }
 
-  // TODO T83483191: should we use std::move here?
-  auto map = MapBuffer(buffer, bufferSize);
+  auto map = MapBuffer(std::move(buffer));
 
   // TODO T83483191: we should invalidate the class once the build() method is
   // called.

ReactCommon/react/renderer/mapbuffer/tests/MapBufferTest.cpp

@@ -108,6 +108,18 @@ TEST(MapBufferTest, testUTFStringEntry) {
   EXPECT_EQ(map.getString(0), "Let's count: 的, 一, 是");
 }
 
+TEST(MapBufferTest, testEmojiStringEntry) {
+  auto builder = MapBufferBuilder();
+
+  builder.putString(
+      0, "Let's count: 1️⃣, 2️⃣, 3️⃣, 🤦🏿‍♀️");
+  auto map = builder.build();
+
+  EXPECT_EQ(
+      map.getString(0),
+      "Let's count: 1️⃣, 2️⃣, 3️⃣, 🤦🏿‍♀️");
+}
+
 TEST(MapBufferTest, testUTFStringEntries) {
   auto builder = MapBufferBuilder();