On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
> > But there was one item I wanted to talk about related to the patch. I
> > agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is
> the
> > right general path,
>
> Are you sure about this? Property values can be binary data (so not
> representable as 'str') but are usually represented by «svn_string_t *»;
> for example, svn_repos_parse_fns3_t::set_node_property().
>
Ah yes, I suppose this also has bearing on the discussion in the other
thread relating
to returning property values. With that in mind, perhaps your suggestion
of effectively
defaulting to always being Bytes for char * , svn_string_t, and
svn_stringbuf_t unless there
is a specific circumstance not to makes the most sense as the general
principle.
Yasuhito, I suppose that means we should probably tweak the typemaps to
follow this
principle. Have you already started down that path based on the properties
discussion?
> > but after the patch there is now a "svn_stringbuf_t
> > *output" typemap that is still using "PyStr" instead of "PyBytes".
> > Further, the only usage of "svn_stringbuf_t *output" is in
> > "svn_fs_print_modules" and "svn_ra_print_modules", where the function
> > parameter is actually an output parameter, so the appropriate typemap is
> > probably "argout" instead of "in".
> >
> > Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
> > your patch and perhaps address the issue in the last paragraph then I can
> > commit your patch to swig-py3. I'll also start working more on the
> swig-py3
> > branch to get it to the point that we can actually get this branch merged
> > into trunk finally.
>
> Great. :-)
>
> Cheers,
>
> Daniel
>
Troy
Received on 2018-12-16 18:09:21 CET