subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@apache.org>
Subject Re: Updating swig-py bindings
Date Wed, 14 Mar 2018 21:58:29 GMT
Branko ─îibej wrote:
> For anyone who wants to work on updating the bindings: note that we have
> a lot of language-specific stuff in there that's a consequence of Swig 1
> not knowing any better back in the day. Most of those typemaps can be
> made language-independent (thus reducing the size of the Swig files)
> with features from Swig 2 and especially 3, which introduced built-in
> constructs for handling various kinds of data structures that we're
> currently mapping separately for each target language.

Hooray! I was hoping that might be the case.

Another thought. Some comments and code in the current bindings seem
to indicate that a few of our C APIs are written in a way that makes
it harder to bind in a 'natural' way. We could consider changing those
C APIs to use idioms that bind better.


Here is something that at first seemed a very trivial update: adding
'const' to apr_array_header_t input parameters. I added 'const' across
all instances in our C API many years ago (and I see just one that lacks
it now: svn_repos_freeze()), so we can update the typemaps and remove
the obsolete comment seen in the following patch:

[[[
Index: subversion/bindings/swig/include/svn_containers.swg
===================================================================
--- subversion/bindings/swig/include/svn_containers.swg	(revision 1826615)
+++ subversion/bindings/swig/include/svn_containers.swg	(working copy)
@@ -639,26 +639,18 @@
 }
 #endif
 
-/* svn_delta_path_driver() mutates its 'paths' argument (by sorting it),
-   despite the fact that it is notionally an input parameter - hence, the
-   lack of 'const' in that one case.
-
-   svn_wc_get_update_editor3() and svn_wc_get_switch_editor3() aren't changing
-   their 'preserved_exts' argument, but it is forwarded to
-   svn_cstring_match_glob_list which also doesn't modify it, but does not have
-   const in its prototype.  */
 %apply const apr_array_header_t *STRINGLIST {
   const apr_array_header_t *args,
   const apr_array_header_t *diff_options,
-  apr_array_header_t *paths,
-  apr_array_header_t *revprops,
+  const apr_array_header_t *paths,
+  const apr_array_header_t *revprops,
   const apr_array_header_t *targets,
-  apr_array_header_t *preserved_exts
+  const apr_array_header_t *preserved_exts
 };
 
 #if defined(SWIGPERL) || defined(SWIGRUBY)
 %apply const apr_array_header_t *STRINGLIST_MAY_BE_NULL {
-  apr_array_header_t *revprops
+  const apr_array_header_t *revprops
 };
 #endif
 
]]]

But then I went on through that file doing the same thing until I came to
one where both input and output typemaps are applied to the same pattern
(in Ruby):

[[[
#ifdef SWIGRUBY
%typemap(in) apr_array_header_t *PROP_LIST
{
  VALUE rb_pool;
  apr_pool_t *pool;

  svn_swig_rb_get_pool(argc, argv, self, &rb_pool, &pool);

  $1 = svn_swig_rb_to_apr_array_prop($input, pool);
}

%typemap(out) apr_array_header_t *PROP_LIST
{
  %append_output(svn_swig_rb_prop_apr_array_to_hash_prop($1));
}

[...]
%apply apr_array_header_t *PROP_LIST {
  apr_array_header_t *wcprop_changes,
  apr_array_header_t *propchanges
};
]]]

Now, a 'wcprop_changes' identifier appears not only as function arguments
(with 'const') but also as a field in 'svn_client_commit_item2_t' (without).

At this point I am for the time being out of my depth -- I don't know enough
to know where to add 'const' and where not.

I'll try to learn more Swig, sooner or later.

- Julian

Mime
View raw message