Use std::unique_ptr and std::shared_ptr. (#514)

ehsannas · web-flow · commit 3a2c07b717a6 · 2021-07-07T12:15:44.000-05:00
* Use std::unique_ptr and std::shared_ptr.

* Can't use std::make_unique for android.

* Don't use rvalue reference for unique_ptr.

* Address code review comments.

* Include &lt;utility&gt; for std::forward.
diff --git a/firestore/integration_test_internal/src/android/promise_android_test.cc b/firestore/integration_test_internal/src/android/promise_android_test.cc
@@ -2,12 +2,12 @@
 
 #include "firestore/src/android/promise_android.h"
 
+#include <memory>
 #include <string>
 
 #include "android/cancellation_token_source.h"
 #include "android/firestore_integration_test_android.h"
 #include "android/task_completion_source.h"
-#include "app/memory/unique_ptr.h"
 #include "app/src/assert.h"
 #include "app/src/mutex.h"
 #include "app_framework.h"
@@ -17,6 +17,7 @@
 #include "firestore/src/android/exception_android.h"
 #include "firestore/src/android/firestore_android.h"
 #include "firestore/src/android/promise_factory_android.h"
+#include "firestore/src/common/make_unique.h"
 #include "firestore/src/include/firebase/firestore.h"
 #include "firestore/src/jni/env.h"
 #include "firestore/src/jni/integer.h"
@@ -195,12 +196,12 @@ class TestCompletion : public TestCompletionBase<PublicType, InternalType> {
     if (result == nullptr) {
       result_.reset(nullptr);
     } else {
-      result_ = MakeUnique<PublicType>(*result);
+      result_ = make_unique<PublicType>(*result);
     }
   }
 
  private:
-  UniquePtr<PublicType> result_;
+  std::unique_ptr<PublicType> result_;
 };
 
 // A specialization of `TestCompletionBase` that handles void `PublicType` and
diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc
@@ -4,6 +4,7 @@
 
 #include <cstdlib>
 #include <fstream>
+#include <memory>
 #include <sstream>
 
 #if !defined(__ANDROID__)
@@ -126,13 +127,13 @@ Firestore* FirestoreIntegrationTest::TestFirestore(
 
   App* app = GetApp(name.c_str());
   if (apps_.find(app) == apps_.end()) {
-    apps_[app] = UniquePtr<App>(app);
+    apps_[app] = std::unique_ptr<App>(app);
   }
 
   // Firestore::set_log_level(LogLevel::kLogLevelDebug);
 
   Firestore* db = new Firestore(CreateTestFirestoreInternal(app));
-  firestores_[db] = FirestoreInfo(name, UniquePtr<Firestore>(db));
+  firestores_[db] = FirestoreInfo(name, std::unique_ptr<Firestore>(db));
 
   LocateEmulator(db);
   InitializeFirestore(db);
diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h
@@ -6,11 +6,11 @@
 #include <cstdio>
 #include <iostream>
 #include <map>
+#include <memory>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
-#include "app/memory/unique_ptr.h"
 #include "app/meta/move.h"
 #include "app/src/assert.h"
 #include "app/src/include/firebase/internal/common.h"
@@ -331,23 +331,23 @@ class FirestoreIntegrationTest : public testing::Test {
   class FirestoreInfo {
    public:
     FirestoreInfo() = default;
-    FirestoreInfo(const std::string& name, UniquePtr<Firestore>&& firestore)
-        : name_(name), firestore_(Move(firestore)) {}
+    FirestoreInfo(const std::string& name, std::unique_ptr<Firestore> firestore)
+        : name_(name), firestore_(std::move(firestore)) {}
 
     const std::string& name() const { return name_; }
     Firestore* firestore() const { return firestore_.get(); }
     void ReleaseFirestore() { firestore_.release(); }
 
    private:
     std::string name_;
-    UniquePtr<Firestore> firestore_;
+    std::unique_ptr<Firestore> firestore_;
   };
 
   // The Firestore and App instance caches.
   // Note that `firestores_` is intentionally ordered *after* `apps_` so that
   // the Firestore pointers will be deleted before the App pointers when this
   // object is destructed.
-  mutable std::unordered_map<App*, UniquePtr<App>> apps_;
+  mutable std::unordered_map<App*, std::unique_ptr<App>> apps_;
   mutable std::unordered_map<Firestore*, FirestoreInfo> firestores_;
 };
 
diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc
@@ -3,6 +3,7 @@
 #include "firebase/firestore.h"
 
 #include <algorithm>
+#include <memory>
 
 #if !defined(__ANDROID__)
 #include <future>  // NOLINT(build/c++11)
@@ -18,7 +19,6 @@
 #include "firestore/src/jni/task.h"
 #endif  // defined(__ANDROID__)
 
-#include "app/memory/unique_ptr.h"
 #include "app/src/mutex.h"
 #include "auth/src/include/firebase/auth.h"
 #include "firestore/src/common/macros.h"
@@ -1456,7 +1456,7 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) {
   EXPECT_NE(app, nullptr);
 
   InitResult init_result;
-  auto auth = UniquePtr<Auth>(Auth::GetAuth(app, &init_result));
+  auto auth = std::unique_ptr<Auth>(Auth::GetAuth(app, &init_result));
 #if defined(__ANDROID__)
   if (init_result != kInitResultSuccess) {
     // On Android, it's possible for the Auth library built at head to be too
@@ -1471,7 +1471,8 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) {
   ASSERT_EQ(init_result, kInitResultSuccess);
 #endif
 
-  auto db = UniquePtr<Firestore>(Firestore::GetInstance(app, &init_result));
+  auto db =
+      std::unique_ptr<Firestore>(Firestore::GetInstance(app, &init_result));
   EXPECT_EQ(init_result, kInitResultSuccess);
 
   // Performing a write will initialize Firestore's worker and get the current
diff --git a/firestore/integration_test_internal/src/jni/env_test.cc b/firestore/integration_test_internal/src/jni/env_test.cc
@@ -1,12 +1,14 @@
 // Copyright 2021 Google LLC
 
+#include <memory>
+
 #include "firestore/src/jni/env.h"
 
 #include "Firestore/core/src/util/firestore_exceptions.h"
-#include "app/memory/unique_ptr.h"
 #include "app/meta/move.h"
 #include "firestore/src/android/exception_android.h"
 #include "firestore/src/common/macros.h"
+#include "firestore/src/common/make_unique.h"
 #include "firestore/src/jni/array.h"
 #include "firestore_integration_test.h"
 #include "gtest/gtest.h"
@@ -17,7 +19,7 @@ namespace jni {
 
 class EnvTest : public FirestoreIntegrationTest {
  public:
-  EnvTest() : env_(MakeUnique<Env>(GetEnv())) {}
+  EnvTest() : env_(make_unique<Env>(GetEnv())) {}
 
   ~EnvTest() override {
     // Ensure that after the test is done that any pending exception is cleared
@@ -32,7 +34,7 @@ class EnvTest : public FirestoreIntegrationTest {
   // compiler to propagate this into `EnvTest`'s destructor, but this conflicts
   // with the declaration of the parent class. Holding `Env` with a unique
   // pointer sidesteps this restriction.
-  UniquePtr<Env> env_;
+  std::unique_ptr<Env> env_;
 };
 
 #if FIRESTORE_HAVE_EXCEPTIONS
diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc
@@ -278,11 +278,11 @@ void FieldValueInternal::EnsureCachedBlob(Env& env) const {
   Local<Array<uint8_t>> bytes = blob.ToBytes(env);
   size_t size = bytes.Size(env);
 
-  auto result = MakeShared<std::vector<uint8_t>>(size);
+  auto result = std::make_shared<std::vector<uint8_t>>(size);
   env.GetArrayRegion(bytes, 0, size, &(result->front()));
 
   if (env.ok()) {
-    cached_blob_ = Move(result);
+    cached_blob_ = std::move(result);
   }
 }
 
diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h
@@ -4,9 +4,9 @@
 #define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_FIELD_VALUE_ANDROID_H_
 
 #include <cstdint>
+#include <memory>
 #include <string>
 
-#include "app/memory/shared_ptr.h"
 #include "firebase/firestore/geo_point.h"
 #include "firebase/firestore/timestamp.h"
 #include "firestore/src/include/firebase/firestore/document_reference.h"
@@ -101,7 +101,7 @@ class FieldValueInternal {
   // Below are cached type information. It is costly to get type info from
   // jobject of Object type. So we cache it if we have already known.
   mutable Type cached_type_ = Type::kNull;
-  mutable SharedPtr<std::vector<uint8_t>> cached_blob_;
+  mutable std::shared_ptr<std::vector<uint8_t>> cached_blob_;
 };
 
 inline jni::Object ToJava(const FieldValue& value) {
diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc
@@ -40,6 +40,7 @@
 #include "firestore/src/android/transaction_android.h"
 #include "firestore/src/android/wrapper.h"
 #include "firestore/src/android/write_batch_android.h"
+#include "firestore/src/common/make_unique.h"
 #include "firestore/src/include/firebase/firestore.h"
 #include "firestore/src/jni/array.h"
 #include "firestore/src/jni/array_list.h"
@@ -263,7 +264,7 @@ FirestoreInternal::FirestoreInternal(App* app) {
   FIREBASE_ASSERT(java_user_callback_executor.get() != nullptr);
   user_callback_executor_ = java_user_callback_executor;
 
-  promises_ = MakeUnique<PromiseFactory<AsyncFn>>(this);
+  promises_ = make_unique<PromiseFactory<AsyncFn>>(this);
 }
 
 /* static */
diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h
@@ -6,9 +6,9 @@
 #include <cstdint>
 #include <functional>
 #include <list>
+#include <memory>
 #include <unordered_set>
 
-#include "app/memory/unique_ptr.h"
 #include "app/src/cleanup_notifier.h"
 #include "app/src/future_manager.h"
 #include "app/src/include/firebase/app.h"
@@ -192,7 +192,7 @@ class FirestoreInternal {
   std::list<LambdaEventListener<LoadBundleTaskProgress>> bundle_listeners_;
 
   FutureManager future_manager_;
-  UniquePtr<PromiseFactory<AsyncFn>> promises_;
+  std::unique_ptr<PromiseFactory<AsyncFn>> promises_;
 
   CleanupNotifier cleanup_;
 };
diff --git a/firestore/src/android/promise_android.h b/firestore/src/android/promise_android.h
@@ -4,15 +4,16 @@
 #define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_PROMISE_ANDROID_H_
 
 #include <jni.h>
+#include <memory>
 
-#include "app/memory/unique_ptr.h"
 #include "app/src/reference_counted_future_impl.h"
 #include "app/src/util_android.h"
 #include "firestore/src/android/converter_android.h"
 #include "firestore/src/android/document_snapshot_android.h"
 #include "firestore/src/android/exception_android.h"
 #include "firestore/src/android/firestore_android.h"
 #include "firestore/src/android/query_snapshot_android.h"
+#include "firestore/src/common/make_unique.h"
 #include "firestore/src/jni/env.h"
 #include "firestore/src/jni/object.h"
 #include "firestore/src/jni/task.h"
@@ -76,7 +77,7 @@ class Promise {
   Promise(ReferenceCountedFutureImpl* impl,
           FirestoreInternal* firestore,
           Completion* completion)
-      : completer_(MakeUnique<Completer<PublicType, InternalType>>(
+      : completer_(make_unique<Completer<PublicType, InternalType>>(
             impl, firestore, completion)),
         impl_(impl) {}
 
@@ -191,7 +192,7 @@ class Promise {
     }
   }
 
-  UniquePtr<Completer<PublicType, InternalType>> completer_;
+  std::unique_ptr<Completer<PublicType, InternalType>> completer_;
 
   // Keep these values separate from the Completer in case completion happens
   // before the future is constructed.
diff --git a/firestore/src/android/transaction_android.cc b/firestore/src/android/transaction_android.cc
@@ -179,12 +179,12 @@ void TransactionInternal::PreserveException(jni::Env& env,
   if (ExceptionInternal::IsAnyExceptionThrownByFirestore(env, exception)) {
     exception = ExceptionInternal::Wrap(env, Move(exception));
   }
-  *first_exception_ = Move(exception);
+  *first_exception_ = std::move(exception);
 }
 
 Local<Throwable> TransactionInternal::ClearExceptionOccurred() {
   if (!*first_exception_) return {};
-  return Move(*first_exception_);
+  return std::move(*first_exception_);
 }
 
 Local<Object> TransactionInternal::Create(Env& env,
diff --git a/firestore/src/android/transaction_android.h b/firestore/src/android/transaction_android.h
@@ -3,9 +3,9 @@
 #ifndef FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_TRANSACTION_ANDROID_H_
 #define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_ANDROID_TRANSACTION_ANDROID_H_
 
+#include <memory>
 #include <string>
 
-#include "app/memory/shared_ptr.h"
 #include "app/meta/move.h"
 #include "app/src/embedded_file.h"
 #include "firestore/src/android/wrapper.h"
@@ -26,14 +26,14 @@ class TransactionInternal : public Wrapper {
 
   TransactionInternal(FirestoreInternal* firestore, const jni::Object& obj)
       : Wrapper(firestore, obj),
-        first_exception_(MakeShared<jni::Local<jni::Throwable>>()) {}
+        first_exception_(std::make_shared<jni::Local<jni::Throwable>>()) {}
 
   TransactionInternal(const TransactionInternal& rhs)
       : Wrapper(rhs), first_exception_(rhs.first_exception_) {}
 
   TransactionInternal(TransactionInternal&& rhs)
       : Wrapper(firebase::Move(rhs)),
-        first_exception_(Move(rhs.first_exception_)) {}
+        first_exception_(std::move(rhs.first_exception_)) {}
 
   void Set(const DocumentReference& document,
            const MapFieldValue& data,
@@ -78,7 +78,7 @@ class TransactionInternal : public Wrapper {
   // mechanism to properly handle native calls via JNI. The first exception is
   // shared by a transaction and its copies. User is allowed to make copy and
   // call transaction operation on the copy.
-  SharedPtr<jni::Local<jni::Throwable>> first_exception_;
+  std::shared_ptr<jni::Local<jni::Throwable>> first_exception_;
 };
 
 }  // namespace firestore
diff --git a/firestore/src/common/make_unique.h b/firestore/src/common/make_unique.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright 2021 Google
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Note: This is needed since std::make_unique is only available starting C++14.
+// TODO(b/191981857): Remove this file when possible.
+
+#ifndef FIREBASE_FIRESTORE_CLIENT_CPP_SRC_COMMON_MAKE_UNIQUE_H_
+#define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_COMMON_MAKE_UNIQUE_H_
+
+#include <memory>
+#include <utility>
+
+namespace firebase {
+namespace firestore {
+
+template <typename T, typename... Args>
+std::unique_ptr<T> make_unique(Args&&... args) {
+  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
+}  // namespace firestore
+}  // namespace firebase
+
+#endif  // FIREBASE_FIRESTORE_CLIENT_CPP_SRC_COMMON_MAKE_UNIQUE_H_
diff --git a/firestore/src/jni/loader.h b/firestore/src/jni/loader.h
@@ -154,8 +154,7 @@ class Loader {
 
   bool ok_ = true;
 
-  // A list of classes that were successfully loaded. This is held as a
-  // UniquePtr to allow Loader to be move-only when built with STLPort.
+  // A list of classes that were successfully loaded.
   std::vector<jclass> loaded_classes_;
 
   // A list of embedded files from which to load classes

Original file line number	Diff line number	Diff line change
`@@ -278,11 +278,11 @@ void FieldValueInternal::EnsureCachedBlob(Env& env) const {`
`278`	`278`	`Local<Array<uint8_t>> bytes = blob.ToBytes(env);`
`279`	`279`	`size_t size = bytes.Size(env);`
`280`	`280`
`281`		`- auto result = MakeShared<std::vector<uint8_t>>(size);`
	`281`	`+ auto result = std::make_shared<std::vector<uint8_t>>(size);`
`282`	`282`	`env.GetArrayRegion(bytes, 0, size, &(result->front()));`
`283`	`283`
`284`	`284`	`if (env.ok()) {`
`285`		`- cached_blob_ = Move(result);`
	`285`	`+ cached_blob_ = std::move(result);`
`286`	`286`	`}`
`287`	`287`	`}`
`288`	`288`