From 1425387570d5559ad0e82bd690b0fcc424911ca1 Mon Sep 17 00:00:00 2001
From: Yu Yang <yuyang18@baidu.com>
Date: Wed, 16 Aug 2017 15:52:48 +0800
Subject: [PATCH] Using unique_ptr instead of raw ptr

Fit google C++ style
---
 paddle/framework/operator.h       | 10 ++++++----
 paddle/framework/operator_test.cc |  3 +--
 paddle/operators/net_op.cc        |  6 +++---
 paddle/operators/net_op.h         |  3 ++-
 paddle/operators/net_op_test.cc   |  5 ++---
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/paddle/framework/operator.h b/paddle/framework/operator.h
index 4a1dee6fb00..9e8aef6f856 100644
--- a/paddle/framework/operator.h
+++ b/paddle/framework/operator.h
@@ -112,8 +112,8 @@ class OperatorBase {
   const AttributeMap& Attrs() const { return attrs_; }
 
   // Return a new operator instance, which is as same as this.
-  // NOTE: It is caller's responsibility to delete that operator instance.
-  virtual OperatorBase* Clone() const = 0;
+  // Use unique_ptr to prevent caller forget to delete this pointer.
+  virtual std::unique_ptr<OperatorBase> Clone() const = 0;
 
  public:
   std::string type_;
@@ -132,8 +132,10 @@ class OperatorBase {
 // Macro for define a clone method.
 // If you are writing an kernel operator, `Clone` will be defined when you
 // register it.
-#define DEFINE_OP_CLONE_METHOD(CLS) \
-  OperatorBase* Clone() const final { return new CLS(*this); }
+#define DEFINE_OP_CLONE_METHOD(CLS)                       \
+  std::unique_ptr<OperatorBase> Clone() const final {     \
+    return std::unique_ptr<OperatorBase>(new CLS(*this)); \
+  }
 
 // Macro for define a default constructor for Operator.
 // You can also use
diff --git a/paddle/framework/operator_test.cc b/paddle/framework/operator_test.cc
index ceba7f5e6ea..88362171265 100644
--- a/paddle/framework/operator_test.cc
+++ b/paddle/framework/operator_test.cc
@@ -257,7 +257,6 @@ class OperatorClone : public paddle::framework::OperatorBase {
 
 TEST(Operator, Clone) {
   OperatorClone a("ABC", {}, {}, {});
-  auto* b = a.Clone();
+  auto b = a.Clone();
   ASSERT_EQ(a.Type(), b->Type());
-  delete b;
 }
\ No newline at end of file
diff --git a/paddle/operators/net_op.cc b/paddle/operators/net_op.cc
index 896550f9d0c..77eb07e2f90 100644
--- a/paddle/operators/net_op.cc
+++ b/paddle/operators/net_op.cc
@@ -85,13 +85,13 @@ NetOp::NetOp(const std::string& type,
              const framework::OperatorBase::VarNameMap& inputs,
              const framework::OperatorBase::VarNameMap& outputs,
              const framework::AttributeMap& attrs)
-    : OperatorBase(type, inputs, outputs, attrs) {}
+    : framework::OperatorBase(type, inputs, outputs, attrs) {}
 
-framework::OperatorBase* NetOp::Clone() const {
+std::unique_ptr<framework::OperatorBase> NetOp::Clone() const {
   PADDLE_ENFORCE(
       add_op_done_,
       "Must clone a sealed NetOp, invoke Net::CompleteAddOp before clone");
-  return new NetOp(*this);
+  return std::unique_ptr<OperatorBase>(new NetOp(*this));
 }
 
 }  // namespace operators
diff --git a/paddle/operators/net_op.h b/paddle/operators/net_op.h
index deee543065a..743f0e67dbe 100644
--- a/paddle/operators/net_op.h
+++ b/paddle/operators/net_op.h
@@ -109,7 +109,8 @@ class NetOp : public framework::OperatorBase {
 
   bool IsNetOp() const override;
   std::vector<std::string> OutputVars(bool has_intermediate) const override;
-  framework::OperatorBase* Clone() const override;
+
+  std::unique_ptr<framework::OperatorBase> Clone() const override;
 
   std::vector<std::shared_ptr<OperatorBase>> ops_;
 
diff --git a/paddle/operators/net_op_test.cc b/paddle/operators/net_op_test.cc
index 40e43f46df7..6d6f8bd3548 100644
--- a/paddle/operators/net_op_test.cc
+++ b/paddle/operators/net_op_test.cc
@@ -84,14 +84,13 @@ TEST(NetOp, Clone) {
   net.AddOp(std::shared_ptr<EmptyOp>(new EmptyOp{"empty", {}, {}, {}}));
   net.AddOp(std::shared_ptr<EmptyOp>(new EmptyOp{"empty2", {}, {}, {}}));
   net.CompleteAddOp(true);
-  auto* new_net_op = net.Clone();
+  auto new_net_op = net.Clone();
   ASSERT_NE(new_net_op, nullptr);
   ASSERT_TRUE(new_net_op->IsNetOp());
-  auto* new_net = static_cast<NetOp*>(new_net_op);
+  auto* new_net = static_cast<NetOp*>(new_net_op.get());
   ASSERT_EQ(2, new_net->ops_.size());
   ASSERT_EQ(new_net->ops_[0]->Type(), "empty");
   ASSERT_EQ(new_net->ops_[1]->Type(), "empty2");
-  delete new_net;
 }
 
 }  // namespace operators
-- 
GitLab