Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Thu, Jan 31, 2013 at 4:43 AM, roderich.schupp@gmail.com
<ro...@gmail.com> wrote:
> while trying to use svn_wc_parse_externals_description3 from Perl I
> stumbled over a fex things.
> Patches below are against trunk.

First of all thanks for the patches.

> perl-bindings-1.patch
>
> [[[
> Make svn_wc_parse_externals_description3 available from Perl bindings.
>
> * subversion/bindings/swig/include/svn_containers.swg:
>   Add output typemap for APR array of svn_wc_external_item2_t.
>
> * subversion/bindings/swig/perl/native/Wc.pm:
>   Document function svn_wc_parse_externals_description3.
>   Add the magic to access struct svn_wc_external_item2_t
>   as an object and document its methods.
>
> * subversion/bindings/swig/perl/native/Core.pm:
>   Fix a typo that prevented the use of
>   _p_svn_opt_revision_value_t objects.
> ]]]

Applied unmodified in r1443783.

For future reference though if you're adding support for a function
it'd be nice to add tests for it.  I added some tests in r1443788.  I
realize there's a lot of things that aren't tested in there now.

But if you provide tests it'll speed my ability to apply it since I
don't have to write them before I can apply your changes.

> perl-bindings-2.patch
>
> [[[
> Replace inline typemap with a function.
>
> * subversion/bindings/swig/include/svn_types.swg:
>   The bulky input typemap for svn_opt_revision_t is inlined
>   by Swig for each use in the generated Perl bindings.
>   Move its body ...
>
> * subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c
>   (svn_swig_pl_set_revision): ... to a new function here.
>
> * subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.h:
>   Add function prototype.
> ]]]

Committed in r1443811 with a fix to the croak on the missing closing
brace not having the input arg for the %s in the format string.  Also
adjusted the commit message to mention the improvement of the error
messages.

Wish I could write tests for this but I can't because we croak.  But
that's a long standing problem.

> perl-bindings-3.patch
>
> [[[
> Documentation fix for SVN::Ra.
>
> * subversion/bindings/swig/perl/native/Ra.pm
>   Fix examples: the $path parameter for SVN::Ra::get_dir() and
>   SVN::Ra::get_file() must not start with a slash.
> ]]]

Applied unmodified in r1443813.

Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Fri, Feb 15, 2013 at 11:41 AM, Ben Reser <be...@reser.org> wrote:
> I wouldn't bother to do the testing like this.  You can create an
> _p_svn_opt_revision_t without needing to get it from something like
> svn_wc_parse_externals_description3().  e.g.
>
> my $rev = SVN::_Core::new_svn_opt_revision_t();
> $rev->kind($SVN::Core::opt_revision_number);
> $rev->value->number(1445267);
>
> In which case we can put the test code in the 3client.t tests, which
> already has a repo.

Done in r1446761.

> This is probably a good change to have more complete coverage of
> testing that externals code.  I'll make this change irrespective of
> the other changes.

Done in r1446753.

Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Sun, Feb 17, 2013 at 2:53 PM, Roderich Schupp
<ro...@gmail.com> wrote:
> I used swig 2.0.9, but the change happened from 2.0.4 to 2.0.5.
> A quick "git bisect" puts the blame on
> https://github.com/swig/swig/commit/30206f975c3c3fbd6e702499727375b4321b490e
> "Fix constructors in named typedef class declarations".
> Probably an unintended side effect, though IMHO the behaviour in 2.0.9 is
> correct.

Agreed I do think the newer behavior is better, nor is it a problem to
support, so I'm not going to worry about it.

Thanks for the patches and finding the exact change that caused the
behavior change.

Re: [PATCH] Small fixes to the Perl bindings

[Roderich Schupp <ro...@gmail.com>]

On Fri, Feb 15, 2013 at 8:41 PM, Ben Reser <be...@reser.org> wrote:

> I flipped through the changelog for SWIG and I didn't see anything
> that explains this difference in behavior.
>


I used swig 2.0.9, but the change happened from 2.0.4 to 2.0.5.
A quick "git bisect" puts the blame on
https://github.com/swig/swig/commit/30206f975c3c3fbd6e702499727375b4321b490e
"Fix constructors in named typedef class declarations".
Probably an unintended side effect, though IMHO the behaviour in 2.0.9 is
correct.

Cheers, Roderich

Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Fri, Feb 15, 2013 at 12:58 AM, Roderich Schupp
<ro...@gmail.com> wrote:
> Err, no. The accessor functions don't go through the typemap, only
> the wrappers for "real" functions do that (add a printf() or warn() at the
> start of  svn_swig_pl_set_revision() to see when it's called).

I guess this depends on the SWIG version.  I went to copy the code of
_wrap_svn_opt_revision_t_kind_get() out of
subversion/bindings/swig/perl/native/core.c to show you this and
couldn't find the call.  I was doing it off my laptop and was very
surprised.  My laptop has SWIG 2.0.8 installed.  So I looked on my
Ubuntu 12.04 VM that I did this work on and found that it had SWIG
2.0.4 (provided via the Ubuntu package) and indeed the
_wrap_svn_opt_revision_t_kind_get() looks looks like this:

[[[
XS(_wrap_svn_opt_revision_t_kind_get) {
  {
    svn_opt_revision_t *arg1 = (svn_opt_revision_t *) 0 ;
    svn_opt_revision_t rev1 ;
    int argvi = 0;
    enum svn_opt_revision_kind result;
    dXSARGS;

    if ((items < 1) || (items > 1)) {
      SWIG_croak("Usage: svn_opt_revision_t_kind_get(self);");
    }
    {
      arg1 = &rev1;
      svn_swig_pl_set_revision(&rev1, ST(0));
    }
    result = (enum svn_opt_revision_kind) ((arg1)->kind);
    ST(argvi) = SWIG_From_int  SWIG_PERL_CALL_ARGS_1((int)(result)); argvi++ ;

    XSRETURN(argvi);
  fail:

    SWIG_croak_null();
  }
}
]]]

This probably explains why you couldn't reproduce the problem I found.

I flipped through the changelog for SWIG and I didn't see anything
that explains this difference in behavior.

> So the ultimate test that the typemap does the right thing for a
> _p_svn_opt_revision_t object is to pass one into a wrapped function
> with a svn_opt_revision_t* parameter (on the C level). Unfortunately
> any such function needs at least a repository to test.
>
> The attached patch adds a test passing an _p_svn_opt_revision_t
> into SVN::Client::log2 (ie. the wrapper for svn_client_log2).

I wouldn't bother to do the testing like this.  You can create an
_p_svn_opt_revision_t without needing to get it from something like
svn_wc_parse_externals_description3().  e.g.

my $rev = SVN::_Core::new_svn_opt_revision_t();
$rev->kind($SVN::Core::opt_revision_number);
$rev->value->number(1445267);

In which case we can put the test code in the 3client.t tests, which
already has a repo.

Note that's calling the wrapper function to create a new
_p_svn_opt_revision_t directly.  We could and probably should produce
a nice clean Perlish wrapper that uses this.

> Note that I slightly modified the svn:externals string in the existing test
> case
> in order to obtain _p_svn_opt_revision_t objects of kinds "head" and
> "number".

This is probably a good change to have more complete coverage of
testing that externals code.  I'll make this change irrespective of
the other changes.

Re: [PATCH] Small fixes to the Perl bindings

[Roderich Schupp <ro...@gmail.com>]

On Wed, Feb 13, 2013 at 5:28 PM, Ben Reser <be...@reser.org> wrote:

> ... Retrieving values out of a
> svn_opt_revision_t object would have exercised the issue since you're
> calling the generated accessor function
>

Err, no. The accessor functions don't go through the typemap, only
the wrappers for "real" functions do that (add a printf() or warn() at the
start of  svn_swig_pl_set_revision() to see when it's called).

So the ultimate test that the typemap does the right thing for a
_p_svn_opt_revision_t object is to pass one into a wrapped function
with a svn_opt_revision_t* parameter (on the C level). Unfortunately
any such function needs at least a repository to test.

The attached patch adds a test passing an _p_svn_opt_revision_t
into SVN::Client::log2 (ie. the wrapper for svn_client_log2).
Note that I slightly modified the svn:externals string in the existing test
case
in order to obtain _p_svn_opt_revision_t objects of kinds "head" and
"number".

Cheers, Roderich

Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Wed, Feb 13, 2013 at 6:57 AM, Roderich Schupp
<ro...@gmail.com> wrote:
> Ouch. Your diagnosis is correct, though I can't reproduce your symptoms :)
> The previously failing tests in t/9wc.t do not cover the problematic
> conversion.
> For that, you would have to pass a Perl _svn_opt_opt_revision_t object
> (e.g. obtained from the return value of
> SVN::Wc::parse_externals_description3)
> into a wrapped function expecting a C *svn_opt_revision_t, e.g.
> SVN::Client::log.

Actually you don't even have to do that.  Retrieving values out of a
svn_opt_revision_t object would have exercised the issue since you're
calling the generated accessor function (e.g.
_wrap_svn_opt_revision_t_kind_get()) which is taking the
svn_opt_revision_t object which SWIG applies the typemap to.

> Anyway, the pointer to pointer thing is ugly. What about the following patch
> that makes it more explicit that the converted argument may be assigned to?

Thanks that is cleaner.  Applied in r1445705.

Re: [PATCH] Small fixes to the Perl bindings

[Roderich Schupp <ro...@gmail.com>]

On Tue, Feb 12, 2013 at 6:06 PM, Ben Reser <be...@reser.org> wrote:

> FYI you may want to apply the changes in r1445267 to your local copy.
> I found a bug in the conversion of the svn_opt_revision_t typemap to
> use a function.
>


Ouch. Your diagnosis is correct, though I can't reproduce your symptoms :)
The previously failing tests in t/9wc.t do not cover the problematic
conversion.
For that, you would have to pass a Perl _svn_opt_opt_revision_t object
(e.g. obtained from the return value of
SVN::Wc::parse_externals_description3)
into a wrapped function expecting a C *svn_opt_revision_t, e.g.
SVN::Client::log.

Anyway, the pointer to pointer thing is ugly. What about the following patch
that makes it more explicit that the converted argument may be assigned to?

Cheers, Roderich

Re: [PATCH] Small fixes to the Perl bindings

[Ben Reser <be...@reser.org>]

On Fri, Feb 8, 2013 at 1:29 AM, roderich.schupp@gmail.com
<ro...@gmail.com> wrote:
> On Friday, February 8, 2013 1:53:03 AM UTC+1, Ben Reser wrote:
>>
>> For future reference though if you're adding support for a function
>> it'd be nice to add tests for it.
>
>
> I'll keep that in mind. Thanks for applying, Ben.

FYI you may want to apply the changes in r1445267 to your local copy.
I found a bug in the conversion of the svn_opt_revision_t typemap to
use a function.

Re: [PATCH] Small fixes to the Perl bindings

["roderich.schupp@gmail.com" <ro...@gmail.com>]

On Friday, February 8, 2013 1:53:03 AM UTC+1, Ben Reser wrote:
>
> For future reference though if you're adding support for a function 
> it'd be nice to add tests for it. 
>

I'll keep that in mind. Thanks for applying, Ben.

Cheers, Roderich