[ 
https://issues.apache.org/jira/browse/ARROW-2171?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16369378#comment-16369378
 ] 

ASF GitHub Bot commented on ARROW-2171:
---------------------------------------

wesm closed pull request #1626: ARROW-2171: [C++/Python] Make OwnedRef safer
URL: https://github.com/apache/arrow/pull/1626
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/cpp/src/arrow/python/common.h b/cpp/src/arrow/python/common.h
index 269385c1a..b2844b18c 100644
--- a/cpp/src/arrow/python/common.h
+++ b/cpp/src/arrow/python/common.h
@@ -67,7 +67,7 @@ class ARROW_EXPORT PyAcquireGIL {
 class ARROW_EXPORT OwnedRef {
  public:
   OwnedRef() : obj_(NULLPTR) {}
-
+  OwnedRef(OwnedRef&& other) : OwnedRef(other.detach()) {}
   explicit OwnedRef(PyObject* obj) : obj_(obj) {}
 
   ~OwnedRef() { reset(); }
@@ -90,6 +90,8 @@ class ARROW_EXPORT OwnedRef {
   PyObject** ref() { return &obj_; }
 
  private:
+  ARROW_DISALLOW_COPY_AND_ASSIGN(OwnedRef);
+
   PyObject* obj_;
 };
 
@@ -98,6 +100,10 @@ class ARROW_EXPORT OwnedRef {
 // (e.g. if it is released in the middle of a function for performance reasons)
 class ARROW_EXPORT OwnedRefNoGIL : public OwnedRef {
  public:
+  OwnedRefNoGIL() : OwnedRef() {}
+  OwnedRefNoGIL(OwnedRefNoGIL&& other) : OwnedRef(other.detach()) {}
+  explicit OwnedRefNoGIL(PyObject* obj) : OwnedRef(obj) {}
+
   ~OwnedRefNoGIL() {
     PyAcquireGIL lock;
     reset();
diff --git a/cpp/src/arrow/python/python-test.cc 
b/cpp/src/arrow/python/python-test.cc
index bcf89a4f6..a2b832bdb 100644
--- a/cpp/src/arrow/python/python-test.cc
+++ b/cpp/src/arrow/python/python-test.cc
@@ -42,6 +42,40 @@ TEST(PyBuffer, InvalidInputObject) {
   ASSERT_EQ(old_refcnt, Py_REFCNT(input));
 }
 
+TEST(OwnedRef, TestMoves) {
+  PyAcquireGIL lock;
+  std::vector<OwnedRef> vec;
+  PyObject *u, *v;
+  u = PyList_New(0);
+  v = PyList_New(0);
+  {
+    OwnedRef ref(u);
+    vec.push_back(std::move(ref));
+    ASSERT_EQ(ref.obj(), nullptr);
+  }
+  vec.emplace_back(v);
+  ASSERT_EQ(Py_REFCNT(u), 1);
+  ASSERT_EQ(Py_REFCNT(v), 1);
+}
+
+TEST(OwnedRefNoGIL, TestMoves) {
+  std::vector<OwnedRefNoGIL> vec;
+  PyObject *u, *v;
+  {
+    PyAcquireGIL lock;
+    u = PyList_New(0);
+    v = PyList_New(0);
+  }
+  {
+    OwnedRefNoGIL ref(u);
+    vec.push_back(std::move(ref));
+    ASSERT_EQ(ref.obj(), nullptr);
+  }
+  vec.emplace_back(v);
+  ASSERT_EQ(Py_REFCNT(u), 1);
+  ASSERT_EQ(Py_REFCNT(v), 1);
+}
+
 class DecimalTest : public ::testing::Test {
  public:
   DecimalTest() : lock_(), decimal_module_(), decimal_constructor_() {


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> [Python] OwnedRef is fragile
> ----------------------------
>
>                 Key: ARROW-2171
>                 URL: https://issues.apache.org/jira/browse/ARROW-2171
>             Project: Apache Arrow
>          Issue Type: Bug
>          Components: Python
>    Affects Versions: 0.8.0
>            Reporter: Antoine Pitrou
>            Assignee: Antoine Pitrou
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 0.9.0
>
>
> Some uses of OwnedRef can implicitly invoke its (default) copy constructor, 
> which will lead to extraneous decrefs.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to