update_engine: Replace scoped_refptr with shared_ptr in update_manager
It seems like scoped_refptr was a substitute for shared_ptr before
chromium was on C++11:
https://www.chromium.org/developers/smart-pointer-guidelines
But that is not the case anymore as we are already on C++14. So just
replace it in update_manager with shared_ptr.
There is still another use case of it for keeping dbus connections but
that can't easily be changed because brillo::DBusConnection is still
using scoped_refptr.
BUG=chromium:994048
TEST=FEATURES=test emerge update_engine
Change-Id: I1fab0408399d678d2851731aea40fc02be459295
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/1755262
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/update_manager/evaluation_context.h b/update_manager/evaluation_context.h
index c68c430..5c5b013 100644
--- a/update_manager/evaluation_context.h
+++ b/update_manager/evaluation_context.h
@@ -23,7 +23,6 @@
#include <base/bind.h>
#include <base/callback.h>
-#include <base/memory/ref_counted.h>
#include <base/memory/weak_ptr.h>
#include <base/time/time.h>
#include <brillo/message_loops/message_loop.h>
@@ -46,7 +45,7 @@
//
// Example:
//
-// scoped_refptr<EvaluationContext> ec = new EvaluationContext(...);
+// auto ec = std::make_shared<EvaluationContext>(...);
//
// ...
// // The following call to ResetEvaluation() is optional. Use it to reset the
@@ -62,8 +61,7 @@
// // If the provided |closure| wants to re-evaluate the policy, it should
// // call ec->ResetEvaluation() to start a new evaluation.
//
-class EvaluationContext : public base::RefCounted<EvaluationContext>,
- private BaseVariable::ObserverInterface {
+class EvaluationContext : private BaseVariable::ObserverInterface {
public:
EvaluationContext(
chromeos_update_engine::ClockInterface* clock,
diff --git a/update_manager/evaluation_context_unittest.cc b/update_manager/evaluation_context_unittest.cc
index 151b0b5..a50defd 100644
--- a/update_manager/evaluation_context_unittest.cc
+++ b/update_manager/evaluation_context_unittest.cc
@@ -39,6 +39,7 @@
using brillo::MessageLoopRunMaxIterations;
using brillo::MessageLoopRunUntil;
using chromeos_update_engine::FakeClock;
+using std::shared_ptr;
using std::string;
using std::unique_ptr;
using testing::_;
@@ -59,14 +60,14 @@
}
template <typename T>
-void ReadVar(scoped_refptr<EvaluationContext> ec, Variable<T>* var) {
+void ReadVar(shared_ptr<EvaluationContext> ec, Variable<T>* var) {
ec->GetValue(var);
}
// Runs |evaluation|; if the value pointed by |count_p| is greater than zero,
// decrement it and schedule a reevaluation; otherwise, writes true to |done_p|.
void EvaluateRepeatedly(Closure evaluation,
- scoped_refptr<EvaluationContext> ec,
+ shared_ptr<EvaluationContext> ec,
int* count_p,
bool* done_p) {
evaluation.Run();
@@ -92,11 +93,11 @@
fake_clock_.SetMonotonicTime(Time::FromTimeT(1240428300));
// Mar 2, 2006 1:23:45 UTC.
fake_clock_.SetWallclockTime(Time::FromTimeT(1141262625));
- eval_ctx_ = new EvaluationContext(
+ eval_ctx_.reset(new EvaluationContext(
&fake_clock_,
default_timeout_,
default_timeout_,
- unique_ptr<base::Callback<void(EvaluationContext*)>>(nullptr));
+ unique_ptr<base::Callback<void(EvaluationContext*)>>(nullptr)));
}
void TearDown() override {
@@ -126,7 +127,7 @@
brillo::FakeMessageLoop loop_{nullptr};
FakeClock fake_clock_;
- scoped_refptr<EvaluationContext> eval_ctx_;
+ shared_ptr<EvaluationContext> eval_ctx_;
// FakeVariables used for testing the EvaluationContext. These are required
// here to prevent them from going away *before* the EvaluationContext under
diff --git a/update_manager/policy_test_utils.cc b/update_manager/policy_test_utils.cc
index 5491e00..653592a 100644
--- a/update_manager/policy_test_utils.cc
+++ b/update_manager/policy_test_utils.cc
@@ -34,7 +34,8 @@
void UmPolicyTestBase::SetUp() {
loop_.SetAsCurrent();
SetUpDefaultClock();
- eval_ctx_ = new EvaluationContext(&fake_clock_, TimeDelta::FromSeconds(5));
+ eval_ctx_.reset(
+ new EvaluationContext(&fake_clock_, TimeDelta::FromSeconds(5)));
SetUpDefaultState();
}
diff --git a/update_manager/policy_test_utils.h b/update_manager/policy_test_utils.h
index eb5758f..cd94907 100644
--- a/update_manager/policy_test_utils.h
+++ b/update_manager/policy_test_utils.h
@@ -93,7 +93,7 @@
brillo::FakeMessageLoop loop_{nullptr};
chromeos_update_engine::FakeClock fake_clock_;
FakeState fake_state_;
- scoped_refptr<EvaluationContext> eval_ctx_;
+ std::shared_ptr<EvaluationContext> eval_ctx_;
std::unique_ptr<Policy> policy_;
};
diff --git a/update_manager/update_manager-inl.h b/update_manager/update_manager-inl.h
index e9dee3f..a1d172d 100644
--- a/update_manager/update_manager-inl.h
+++ b/update_manager/update_manager-inl.h
@@ -78,7 +78,7 @@
template <typename R, typename... Args>
void UpdateManager::OnPolicyReadyToEvaluate(
- scoped_refptr<EvaluationContext> ec,
+ std::shared_ptr<EvaluationContext> ec,
base::Callback<void(EvalStatus status, const R& result)> callback,
EvalStatus (Policy::*policy_method)(
EvaluationContext*, State*, std::string*, R*, Args...) const,
@@ -119,8 +119,7 @@
EvaluationContext*, State*, std::string*, R*, ExpectedArgs...) const,
R* result,
ActualArgs... args) {
- scoped_refptr<EvaluationContext> ec(
- new EvaluationContext(clock_, evaluation_timeout_));
+ auto ec = std::make_shared<EvaluationContext>(clock_, evaluation_timeout_);
// A PolicyRequest always consists on a single evaluation on a new
// EvaluationContext.
// IMPORTANT: To ensure that ActualArgs can be converted to ExpectedArgs, we
@@ -141,7 +140,7 @@
EvalStatus (Policy::*policy_method)(
EvaluationContext*, State*, std::string*, R*, ExpectedArgs...) const,
ActualArgs... args) {
- scoped_refptr<EvaluationContext> ec = new EvaluationContext(
+ auto ec = std::make_shared<EvaluationContext>(
clock_,
evaluation_timeout_,
expiration_timeout_,
@@ -149,7 +148,7 @@
new base::Callback<void(EvaluationContext*)>(
base::Bind(&UpdateManager::UnregisterEvalContext,
weak_ptr_factory_.GetWeakPtr()))));
- if (!ec_repo_.insert(ec.get()).second) {
+ if (!ec_repo_.insert(ec).second) {
LOG(ERROR) << "Failed to register evaluation context; this is a bug.";
}
diff --git a/update_manager/update_manager.cc b/update_manager/update_manager.cc
index 5664a5c..2974d7d 100644
--- a/update_manager/update_manager.cc
+++ b/update_manager/update_manager.cc
@@ -47,7 +47,11 @@
}
void UpdateManager::UnregisterEvalContext(EvaluationContext* ec) {
- if (!ec_repo_.erase(ec)) {
+ // Since |ec_repo_|'s compare function is based on the value of the raw
+ // pointer |ec|, we can just create a |shared_ptr| here and pass it along to
+ // be erased.
+ if (!ec_repo_.erase(
+ std::shared_ptr<EvaluationContext>(ec, [](EvaluationContext*) {}))) {
LOG(ERROR) << "Unregistering an unknown evaluation context, this is a bug.";
}
}
diff --git a/update_manager/update_manager.h b/update_manager/update_manager.h
index 732175f..8ab61d0 100644
--- a/update_manager/update_manager.h
+++ b/update_manager/update_manager.h
@@ -22,7 +22,6 @@
#include <string>
#include <base/callback.h>
-#include <base/memory/ref_counted.h>
#include <base/time/time.h>
#include "update_engine/common/clock_interface.h"
@@ -33,15 +32,6 @@
namespace chromeos_update_manager {
-// Comparator for scoped_refptr objects.
-template <typename T>
-struct ScopedRefPtrLess {
- bool operator()(const scoped_refptr<T>& first,
- const scoped_refptr<T>& second) const {
- return first.get() < second.get();
- }
-};
-
// Please do not move this class into a new file for simplicity.
// This pure virtual class is purely created for purpose of testing. The reason
// was that |UpdateManager|'s member functions are templatized, which does not
@@ -152,7 +142,7 @@
// the evaluation will be re-scheduled to be called later.
template <typename R, typename... Args>
void OnPolicyReadyToEvaluate(
- scoped_refptr<EvaluationContext> ec,
+ std::shared_ptr<EvaluationContext> ec,
base::Callback<void(EvalStatus status, const R& result)> callback,
EvalStatus (Policy::*policy_method)(
EvaluationContext*, State*, std::string*, R*, Args...) const,
@@ -186,9 +176,7 @@
// destructed; alternatively, when the UpdateManager instance is destroyed, it
// will remove all pending events associated with all outstanding contexts
// (which should, in turn, trigger their destruction).
- std::set<scoped_refptr<EvaluationContext>,
- ScopedRefPtrLess<EvaluationContext>>
- ec_repo_;
+ std::set<std::shared_ptr<EvaluationContext>> ec_repo_;
base::WeakPtrFactory<UpdateManager> weak_ptr_factory_;