[Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Yasuhito FUTATSUKI
Hi, I found SWIP Python bindings of APIs using svn_stream_t * in _client,
_delta, _diff, _fs, _ra and _repos don't accept core.svn_stream_t proxy
objects, so I modified %typemap(in) svn_stream_t *WRAPPED_STRAM to accept
them as well as file like objects.

The patch below is destined to trunk@1848063, contains test using
core.svn_stream_t object, core.Stream() object for svn_stream_t argments.
(test for using file like object and None type is already exists)

--- Start of patch ---
commit 8c438d48c979efb494072864acc0ddb400f4792d
Author: FUTATSUKI Yasuhito <[hidden email]>
Date:   Wed Dec 5 02:46:17 2018 +0900

     On branch improve_swig_py_stream_IF: swig-py: allow svn.core.svn_stream_t
     object for svn_stream_t * interface
     
     * subversion/bindings/swig/include/svn_types.swg
       (%typemap(in) svn_stream_t *WRAPPED_STREAM)
       allow svn.core.svn_stream_t proxy object for svn_stream_t * in args.
       this typemap is used by _client, _delta, _diff, _fs, _ra, and _repos
       modules.

diff --git a/subversion/bindings/swig/include/svn_types.swg b/subversion/bindings/swig/include/svn_types.swg
index 4ad5d1658b..91ebb8dd23 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -941,7 +941,62 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
 
  #ifdef SWIGPYTHON
  %typemap(in) svn_stream_t *WRAPPED_STREAM {
-    $1 = svn_swig_py_make_stream ($input, _global_pool);
+    if ($input == Py_None) {
+        $1 = NULL;
+    }
+    else {
+        PyObject *libsvn_core;
+        PyObject *py_stream_t;
+        libsvn_core = PyImport_ImportModule("libsvn.core");
+        if (PyErr_Occurred()) {
+            Py_XDECREF(libsvn_core);
+            SWIG_fail;
+        }
+        py_stream_t = PyObject_GetAttrString(libsvn_core, "svn_stream_t");
+        if (PyErr_Occurred()) {
+            Py_XDECREF(py_stream_t);
+            Py_DECREF(libsvn_core);
+            SWIG_fail;
+        }
+        if (PyObject_IsInstance($input, py_stream_t)) {
+            $1 = (svn_stream_t *)svn_swig_py_must_get_ptr(
+                                    $input, $1_descriptor, $argnum);
+            if (PyErr_Occurred()) {
+                Py_DECREF(py_stream_t);
+                Py_DECREF(libsvn_core);
+                SWIG_fail;
+            }
+            Py_DECREF(py_stream_t);
+            Py_DECREF(libsvn_core);
+        }
+        else if (PyObject_HasAttrString($input, "_stream")){
+            PyObject *_stream = PyObject_GetAttrString($input, "_stream");
+            if (PyObject_IsInstance(_stream, py_stream_t)) {
+                $1 = (svn_stream_t *)svn_swig_py_must_get_ptr(
+                                        _stream, $1_descriptor,
+                                        $argnum);
+                if (PyErr_Occurred()) {
+                    PyErr_Clear();
+                    $1 = NULL;
+                }
+                Py_DECREF(_stream);
+                Py_DECREF(py_stream_t);
+                Py_DECREF(libsvn_core);
+            }
+        }
+        if ($1 == NULL) {
+            if (   PyObject_HasAttrString($input, "read")
+                || PyObject_HasAttrString($input, "write")) {
+                $1 = svn_swig_py_make_stream ($input, _global_pool);
+            }
+            else {
+                PyErr_SetString(PyExc_TypeError,
+                                "expecting a svn_stream_t"
+                                " or file like object");
+                SWIG_fail;
+            }
+        }
+    }
  }
  #endif
 

commit c757e91aa6e5b30acc2e63bd8c3c3b144b09c1f7
Author: FUTATSUKI Yasuhito <[hidden email]>
Date:   Wed Dec 5 02:18:03 2018 +0900

     On branch improve_swig_py_stream_IF: add unit test svn_stream_t * I/F
     
     * subversion/bindings/swig/python/tests/delta.py
       (DeltaTestCase.testTxWindowHandler_stream_IF): New test for svn_stream_t *
       interface wrapper accept svn.core.svn_stream_t proxy object.
       (DeltaTestCase.testTxWindowHandler_Stream_IF): New test for svn_stream_t *
       interface wrapper accept svn.core.Stream wrapper object.

diff --git a/subversion/bindings/swig/python/tests/delta.py b/subversion/bindings/swig/python/tests/delta.py
index 162cf322de..7f6ba6782f 100644
--- a/subversion/bindings/swig/python/tests/delta.py
+++ b/subversion/bindings/swig/python/tests/delta.py
@@ -19,6 +19,8 @@
  #
  #
  import unittest, setup_path
+import os
+import tempfile
  import svn.delta
  import svn.core
  from sys import version_info # For Python version check
@@ -47,6 +49,59 @@ class DeltaTestCase(unittest.TestCase):
         svn.delta.tx_apply(src_stream, target_stream, None)
      window_handler(None, baton)
 
+  def testTxWindowHandler_stream_IF(self):
+    """Test tx_invoke_window_handler, with svn.core.svn_stream_t object"""
+    pool = svn.core.Pool()
+    in_str = "hello world"
+    src_stream = svn.core.svn_stream_from_stringbuf(in_str)
+    content_str = "bye world"
+    content_stream = svn.core.svn_stream_from_stringbuf(content_str)
+    fd, fname = tempfile.mkstemp()
+    os.close(fd)
+    try:
+      target_stream = svn.core.svn_stream_from_aprfile2(fname, False)
+      window_handler, baton = \
+          svn.delta.tx_apply(src_stream, target_stream, None)
+      svn.delta.tx_send_stream(content_stream, window_handler, baton, pool)
+      fp = open(fname, 'rb')
+      out_str = fp.read()
+      fp.close()
+      self.assertEqual(content_str, out_str)
+    finally:
+      del pool
+      try:
+        os.remove(fname)
+      except OSError:
+        pass
+
+  def testTxWindowHandler_Stream_IF(self):
+    """Test tx_invoke_window_handler, with svn.core.Stream object"""
+    pool = svn.core.Pool()
+    in_str = "hello world"
+    src_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_stringbuf(in_str))
+    content_str = "bye world"
+    content_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_stringbuf(content_str))
+    fd, fname = tempfile.mkstemp()
+    os.close(fd)
+    try:
+      target_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_aprfile2(fname, False))
+      window_handler, baton = \
+          svn.delta.tx_apply(src_stream, target_stream, None)
+      svn.delta.tx_send_stream(content_stream, window_handler, baton, None)
+      fp = open(fname, 'rb')
+      out_str = fp.read()
+      fp.close()
+      self.assertEqual(content_str, out_str)
+    finally:
+      del pool
+      try:
+        os.remove(fname)
+      except OSError:
+        pass
+
    def testTxdeltaWindowT(self):
      """Test the svn_txdelta_window_t wrapper."""
      a = StringIO("abc\ndef\n")

--- END of patch ---
Regards,
--
Yasuthito FUTATSUKI
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Yasuhito FUTATSUKI
On 12/6/18 5:06 PM, Yasuhito FUTATSUKI wrote:
> Hi, I found SWIP Python bindings of APIs using svn_stream_t * in _client,
> _delta, _diff, _fs, _ra and _repos don't accept core.svn_stream_t proxy
> objects, so I modified %typemap(in) svn_stream_t *WRAPPED_STRAM to accept
> them as well as file like objects.

Those who try to use APIs to pass svn_stream_t * from Python may try to
pass core.svn_stream_t object from other API returns it, because as there is
no appropriate document public available to describe what can be acceptable
for those APIs, so it is natural to do so. Actually I found the code below
in ViewVC:

   temp = tempfile.mktemp()
   stream = core.svn_stream_from_aprfile(temp)
   url = svnrepos._geturl(path)
   client.svn_client_cat(core.Stream(stream), url, _rev2optrev(rev),
                         svnrepos.ctx)
   core.svn_stream_close(stream)
   return temp

I think the author of this code try to pass 'stream' to client.svn_client_cat
directly and got exception, then wrap with core.Stream() to avoid it.
That's why I tried to modify this typemap.


Jun Omae kindly reviewed and rewrote my patch to move code to check object
type into svn_swig_py_make_stream() in swigutil_py.c to minimize expansion
of typemap, and added test for parse_fns3_invalid_set_fulltext() in
swigutil_py.c which is affected by modification of svn_swig_py_make_stream().
(https://github.com/jun66j5/subversion/commits/improve_swig_py_stream_IF)
I also add few modification after it, so I repost modified patch.

--- Start of patch ---
diff --git a/subversion/bindings/swig/include/svn_types.swg b/subversion/bindings/swig/include/svn_types.swg
index 4ad5d1658b..ad66cb160c 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -941,7 +941,15 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
 
  #ifdef SWIGPYTHON
  %typemap(in) svn_stream_t *WRAPPED_STREAM {
-    $1 = svn_swig_py_make_stream ($input, _global_pool);
+    if ($input == Py_None) {
+        $1 = NULL;
+    }
+    else {
+        $1 = svn_swig_py_make_stream ($input, _global_pool);
+        if ($1 == NULL) {
+            SWIG_fail;
+        }
+    }
  }
  #endif
 
diff --git a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
index 27ee404269..392190a9f5 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2293,8 +2293,8 @@ static svn_error_t *parse_fn3_set_fulltext(svn_stream_t **stream,
                                             void *node_baton)
  {
    item_baton *ib = node_baton;
-  PyObject *result;
-  svn_error_t *err;
+  PyObject *result = NULL;
+  svn_error_t *err = SVN_NO_ERROR;
 
    svn_swig_py_acquire_py_lock();
 
@@ -2316,14 +2316,17 @@ static svn_error_t *parse_fn3_set_fulltext(svn_stream_t **stream,
        /* create a stream from the IO object. it will increment the
           reference on the 'result'. */
        *stream = svn_swig_py_make_stream(result, ib->pool);
+      if (*stream == NULL)
+        {
+          err = callback_exception_error();
+          goto finished;
+        }
      }
 
+ finished:
    /* if the handler returned an IO object, svn_swig_py_make_stream() has
       incremented its reference counter. If it was None, it is discarded. */
-  Py_DECREF(result);
-  err = SVN_NO_ERROR;
-
- finished:
+  Py_XDECREF(result);
    svn_swig_py_release_py_lock();
    return err;
  }
@@ -2575,17 +2578,59 @@ svn_swig_py_stream_destroy(void *py_io)
  svn_stream_t *
  svn_swig_py_make_stream(PyObject *py_io, apr_pool_t *pool)
  {
-  svn_stream_t *stream;
+  PyObject *libsvn_core = NULL;
+  PyObject *py_stream_t = NULL;
+  PyObject *_stream = NULL;
+  svn_stream_t *result = NULL;
+  swig_type_info *typeinfo = svn_swig_TypeQuery("svn_stream_t *");
 
-  stream = svn_stream_create(py_io, pool);
-  svn_stream_set_read2(stream, read_handler_pyio, NULL);
-  svn_stream_set_write(stream, write_handler_pyio);
-  svn_stream_set_close(stream, close_handler_pyio);
-  apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
-                            apr_pool_cleanup_null);
-  Py_INCREF(py_io);
+  libsvn_core = PyImport_ImportModule("libsvn.core");
+  if (PyErr_Occurred()) {
+    goto finished;
+  }
+  py_stream_t = PyObject_GetAttrString(libsvn_core, "svn_stream_t");
+  if (PyErr_Occurred()) {
+    goto finished;
+  }
+  if (PyObject_IsInstance(py_io, py_stream_t)) {
+    result = (svn_stream_t *)svn_swig_py_must_get_ptr(py_io, typeinfo, 0);
+    if (PyErr_Occurred()) {
+      result = NULL;
+      goto finished;
+    }
+  }
+  else if (PyObject_HasAttrString(py_io, "_stream")) {
+    _stream = PyObject_GetAttrString(py_io, "_stream");
+    if (PyObject_IsInstance(_stream, py_stream_t)) {
+      result = (svn_stream_t *)svn_swig_py_must_get_ptr(_stream, typeinfo, 0);
+      if (PyErr_Occurred()) {
+        result = NULL;
+        goto finished;
+      }
+    }
+  }
+  if (result == NULL) {
+    if (   !PyObject_HasAttrString(py_io, "read")
+        && !PyObject_HasAttrString(py_io, "write")) {
+      PyErr_SetString(PyExc_TypeError,
+                      "expecting a svn_stream_t or file like object");
+      goto finished;
+    }
+    result = svn_stream_create(py_io, pool);
+    svn_stream_set_read2(result, read_handler_pyio, NULL);
+    svn_stream_set_write(result, write_handler_pyio);
+    svn_stream_set_close(result, close_handler_pyio);
+    apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
+                              apr_pool_cleanup_null);
+    Py_INCREF(py_io);
+  }
+
+finished:
+  Py_XDECREF(_stream);
+  Py_XDECREF(py_stream_t);
+  Py_XDECREF(libsvn_core);
 
-  return stream;
+  return result;
  }
 
  PyObject *
diff --git a/subversion/bindings/swig/python/tests/delta.py b/subversion/bindings/swig/python/tests/delta.py
index 162cf322de..7f6ba6782f 100644
--- a/subversion/bindings/swig/python/tests/delta.py
+++ b/subversion/bindings/swig/python/tests/delta.py
@@ -19,6 +19,8 @@
  #
  #
  import unittest, setup_path
+import os
+import tempfile
  import svn.delta
  import svn.core
  from sys import version_info # For Python version check
@@ -47,6 +49,59 @@ class DeltaTestCase(unittest.TestCase):
         svn.delta.tx_apply(src_stream, target_stream, None)
      window_handler(None, baton)
 
+  def testTxWindowHandler_stream_IF(self):
+    """Test tx_invoke_window_handler, with svn.core.svn_stream_t object"""
+    pool = svn.core.Pool()
+    in_str = "hello world"
+    src_stream = svn.core.svn_stream_from_stringbuf(in_str)
+    content_str = "bye world"
+    content_stream = svn.core.svn_stream_from_stringbuf(content_str)
+    fd, fname = tempfile.mkstemp()
+    os.close(fd)
+    try:
+      target_stream = svn.core.svn_stream_from_aprfile2(fname, False)
+      window_handler, baton = \
+          svn.delta.tx_apply(src_stream, target_stream, None)
+      svn.delta.tx_send_stream(content_stream, window_handler, baton, pool)
+      fp = open(fname, 'rb')
+      out_str = fp.read()
+      fp.close()
+      self.assertEqual(content_str, out_str)
+    finally:
+      del pool
+      try:
+        os.remove(fname)
+      except OSError:
+        pass
+
+  def testTxWindowHandler_Stream_IF(self):
+    """Test tx_invoke_window_handler, with svn.core.Stream object"""
+    pool = svn.core.Pool()
+    in_str = "hello world"
+    src_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_stringbuf(in_str))
+    content_str = "bye world"
+    content_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_stringbuf(content_str))
+    fd, fname = tempfile.mkstemp()
+    os.close(fd)
+    try:
+      target_stream = svn.core.Stream(
+                    svn.core.svn_stream_from_aprfile2(fname, False))
+      window_handler, baton = \
+          svn.delta.tx_apply(src_stream, target_stream, None)
+      svn.delta.tx_send_stream(content_stream, window_handler, baton, None)
+      fp = open(fname, 'rb')
+      out_str = fp.read()
+      fp.close()
+      self.assertEqual(content_str, out_str)
+    finally:
+      del pool
+      try:
+        os.remove(fname)
+      except OSError:
+        pass
+
    def testTxdeltaWindowT(self):
      """Test the svn_txdelta_window_t wrapper."""
      a = StringIO("abc\ndef\n")
diff --git a/subversion/bindings/swig/python/tests/repository.py b/subversion/bindings/swig/python/tests/repository.py
index 46580b723a..c185c508c3 100644
--- a/subversion/bindings/swig/python/tests/repository.py
+++ b/subversion/bindings/swig/python/tests/repository.py
@@ -231,6 +231,26 @@ class SubversionRepositoryTestCase(unittest.TestCase):
      # the comparison list gets too long.
      self.assertEqual(dsp.ops[:len(expected_list)], expected_list)
 
+  def test_parse_fns3_invalid_set_fulltext(self):
+    self.cancel_calls = 0
+    def is_cancelled():
+      self.cancel_calls += 1
+      return None
+    class DumpStreamParserSubclass(DumpStreamParser):
+      def set_fulltext(self, node_baton):
+        DumpStreamParser.set_fulltext(self, node_baton)
+        return 42
+    dump_path = os.path.join(os.path.dirname(sys.argv[0]),
+        "trac/versioncontrol/tests/svnrepos.dump")
+    stream = open(dump_path)
+    try:
+      dsp = DumpStreamParserSubclass()
+      ptr, baton = repos.make_parse_fns3(dsp)
+      self.assertRaises(TypeError, repos.parse_dumpstream3,
+                        stream, ptr, baton, False, is_cancelled)
+    finally:
+      stream.close()
+
    def test_get_logs(self):
      """Test scope of get_logs callbacks"""
      logs = []
--- End of patch ---

Regards,
--
Yasuhito FUTATSUKI
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Yasuhito FUTATSUKI
Ah, former patch has already been commited. Thank you.
I'm sorry I didn't watch trunk.

--
Yasuhito FUTATSUKI
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Branko Čibej
On 09.12.2018 17:52, Yasuhito FUTATSUKI wrote:
> Ah, former patch has already been commited. Thank you.
> I'm sorry I didn't watch trunk.

That's fine! And thank you for your contribution.

If you think your second patch is better, please consider updating it so
that it applies to current trunk.

-- Brane

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Michael Pilato
On 12/10/18 7:03 AM, Branko Čibej wrote:

> On 09.12.2018 17:52, Yasuhito FUTATSUKI wrote:
>> Ah, former patch has already been commited. Thank you.
>> I'm sorry I didn't watch trunk.
>
> That's fine! And thank you for your contribution.
>
> If you think your second patch is better, please consider updating it so
> that it applies to current trunk.
>
> -- Brane
>

Agreed!  In fact, I was actually trying to take your first patch and
make the same changes present in the second (because I noticed while
grepping for something that your large block of added code was getting
replicated dozens of times across the generated .c files).  But
mentally, I wasn't top-notch on Friday and ended up bailing on my
attempt and committing your patch as-is instead.

As punishment, I'll get the updated patch reviewed and committed.

-- Mike

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Michael Pilato
In reply to this post by Yasuhito FUTATSUKI
On 12/8/18 3:57 AM, Yasuhito FUTATSUKI wrote:
> On 12/6/18 5:06 PM, Yasuhito FUTATSUKI wrote:
> Jun Omae kindly reviewed and rewrote my patch to move code to check object
> type into svn_swig_py_make_stream() in swigutil_py.c to minimize expansion
> of typemap, and added test for parse_fns3_invalid_set_fulltext() in
> swigutil_py.c which is affected by modification of
> svn_swig_py_make_stream().
> (https://github.com/jun66j5/subversion/commits/improve_swig_py_stream_IF)
> I also add few modification after it, so I repost modified patch.
>

Most of the patch is fine.  Besides some minor code formatting tweaks,
there's only one bit I really changed when committing (as r1848577):

> --- a/subversion/bindings/swig/python/tests/repository.py
> +++ b/subversion/bindings/swig/python/tests/repository.py
> @@ -231,6 +231,26 @@ class SubversionRepositoryTestCase(unittest.TestCase):
>       # the comparison list gets too long.
>       self.assertEqual(dsp.ops[:len(expected_list)], expected_list)
>
> +  def test_parse_fns3_invalid_set_fulltext(self):
> +    self.cancel_calls = 0
> +    def is_cancelled():
> +      self.cancel_calls += 1
> +      return None

These mechanics around cancellation aren't necessary, and I'm guessing
were just copied from other test functions nearby.  I've removed them.

> +    class DumpStreamParserSubclass(DumpStreamParser):
> +      def set_fulltext(self, node_baton):
> +        DumpStreamParser.set_fulltext(self, node_baton)
> +        return 42
> +    dump_path = os.path.join(os.path.dirname(sys.argv[0]),
> +        "trac/versioncontrol/tests/svnrepos.dump")
> +    stream = open(dump_path)
> +    try:
> +      dsp = DumpStreamParserSubclass()
> +      ptr, baton = repos.make_parse_fns3(dsp)
> +      self.assertRaises(TypeError, repos.parse_dumpstream3,
> +                        stream, ptr, baton, False, is_cancelled)

...and of course, changed is_cancelled to None here.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Jun Omae
Hi,

On 2018/12/10 22:37, Michael Pilato wrote:
> Most of the patch is fine.  Besides some minor code formatting tweaks,
> there's only one bit I really changed when committing (as r1848577):

Thanks for the reviewing and tweaking.

Reconsidering the checking instance of svn_stream_t in svn_swig_py_make_stream(),
I think the code is redundant. Using svn_swig_py_convert_ptr() would simplify
the converting the given py_io parameter to a svn_stream_t pointer.

Thoughts?


---- START OF PATCH ----
commit f857f1f529cc0e621a074a48e7c689a0b32d18cf
Author: Jun Omae <[hidden email]>
Date:   Tue Dec 11 19:41:25 2018 +0900

     swig-py: Followup to r1848577, simplify the handling svn_stream_t pointer.

     * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
       (svn_swig_py_make_stream): Use svn_swig_py_convert_ptr() rather than
       checking instance of libsvn.core.svn_stream_t using Python APIs.

diff --git a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
index 93d4cb378..2c90a6a46 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2578,36 +2578,18 @@ svn_swig_py_stream_destroy(void *py_io)
  svn_stream_t *
  svn_swig_py_make_stream(PyObject *py_io, apr_pool_t *pool)
  {
-  PyObject *libsvn_core = NULL;
-  PyObject *py_stream_t = NULL;
    PyObject *_stream = NULL;
-  svn_stream_t *result = NULL;
+  void *result = NULL;
    swig_type_info *typeinfo = svn_swig_TypeQuery("svn_stream_t *");

-  libsvn_core = PyImport_ImportModule("libsvn.core");
-  if (PyErr_Occurred()) {
-    goto finished;
-  }
-  py_stream_t = PyObject_GetAttrString(libsvn_core, "svn_stream_t");
-  if (PyErr_Occurred()) {
-    goto finished;
-  }
-  if (PyObject_IsInstance(py_io, py_stream_t)) {
-    result = (svn_stream_t *)svn_swig_py_must_get_ptr(py_io, typeinfo, 0);
-    if (PyErr_Occurred()) {
-      result = NULL;
-      goto finished;
-    }
-  }
-  else if (PyObject_HasAttrString(py_io, "_stream")) {
-    _stream = PyObject_GetAttrString(py_io, "_stream");
-    if (PyObject_IsInstance(_stream, py_stream_t)) {
-      result = (svn_stream_t *)svn_swig_py_must_get_ptr(_stream, typeinfo, 0);
-      if (PyErr_Occurred()) {
-        result = NULL;
-        goto finished;
+  if (svn_swig_py_convert_ptr(py_io, &result, typeinfo) != 0) {
+      PyErr_Clear();
+      if (PyObject_HasAttrString(py_io, "_stream")) {
+        _stream = PyObject_GetAttrString(py_io, "_stream");
+        if (svn_swig_py_convert_ptr(_stream, &result, typeinfo) != 0) {
+          PyErr_Clear();
+        }
        }
-    }
    }
    if (result == NULL) {
      if (!PyObject_HasAttrString(py_io, "read")
@@ -2627,8 +2609,6 @@ svn_swig_py_make_stream(PyObject *py_io, apr_pool_t *pool)

  finished:
    Py_XDECREF(_stream);
-  Py_XDECREF(py_stream_t);
-  Py_XDECREF(libsvn_core);

    return result;
  }
---- END OF PATCH ----

--
Jun Omae <[hidden email]> (大前 潤)
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

Michael Pilato
On 12/11/18 6:12 AM, Jun Omae wrote:

> On 2018/12/10 22:37, Michael Pilato wrote:
>> Most of the patch is fine.  Besides some minor code formatting tweaks,
>> there's only one bit I really changed when committing (as r1848577):
>
> Thanks for the reviewing and tweaking.
>
> Reconsidering the checking instance of svn_stream_t in
> svn_swig_py_make_stream(),
> I think the code is redundant. Using svn_swig_py_convert_ptr() would
> simplify
> the converting the given py_io parameter to a svn_stream_t pointer.
>
> Thoughts?

I agree that we can avoid doing the same sort of stuff that
SWIG_ConvertPtr() would do on our behalf.  The patch makes our code much
tighter (and easy to read).  Committed as-is in r1848763.  Thanks!