You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2005/08/13 03:19:21 UTC
Re: svn commit: r15700 - trunk/subversion/bindings/swig/perl/native
clkao@tigris.org wrote:
> Author: clkao
> Date: Fri Aug 12 10:44:46 2005
> New Revision: 15700
>
> Modified:
> trunk/subversion/bindings/swig/perl/native/Core.pm
>
> Log:
> Follow-up to r15695, more defined-ness tests.
>
> * perl/native/Core.pm:
> (SVN::Core::Stream::READLINE, getline): Fix bugs that content
> with literal "0" will not be read and returned.
>
> Patch by: Pawel Chmielowski <pr...@civ.pl>
>
>
>
> Modified: trunk/subversion/bindings/swig/perl/native/Core.pm
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/bindings/swig/perl/native/Core.pm?rev=15700&p1=trunk/subversion/bindings/swig/perl/native/Core.pm&p2=trunk/subversion/bindings/swig/perl/native/Core.pm&r1=15699&r2=15700
> ==============================================================================
> --- trunk/subversion/bindings/swig/perl/native/Core.pm (original)
> +++ trunk/subversion/bindings/swig/perl/native/Core.pm Fri Aug 12 10:44:46 2005
> @@ -201,7 +201,7 @@
> my $self = shift;
> *$self->{pool} ||= SVN::Core::pool_create (undef);
> my ($buf, $eof) = *$self->{svn_stream}->readline ($/, *$self->{pool});
> - return undef if $eof && !$buf;
> + return undef if $eof && !length($buf);
As a matter of good Perl style, you don't ever want to return undef. If the
caller calls the function in an array context, then the function will return an
array with a single value of undef, which when you test the array for trueness,
will return true.
So the proper thing to do is just return with no value, which returns undef in
scalar context and an empty array in array context:
return if $eof && !length($buf);
From perldoc -f return
return EXPR
return Returns from a subroutine, "eval", or "do FILE" with the value
given in EXPR. Evaluation of EXPR may be in list, scalar, or
void context, depending on how the return value will be used,
and the context may vary from one execution to the next (see
"wantarray"). If no EXPR is given, returns an empty list in
list context, the undefined value in scalar context, and (of
course) nothing at all in a void context.
> return $eof ? $buf : $buf.$/;
> }
>
> @@ -226,7 +226,8 @@
> return $buf;
> }
> elsif (ref $/) {
> - return *$self->{svn_stream}->read (${$/}) || undef;
> + my $buf = *$self->{svn_stream}->read (${$/});
> + return length($buf) ? $buf : undef;
Here you want to do
if (length($buf)) {
return $buf;
} else {
return;
}
Regards,
Blair
--
Blair Zajac, Ph.D.
<bl...@orcaware.com>
Subversion and Orca training and consulting
http://www.orcaware.com/svn/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15700 - trunk/subversion/bindings/swig/perl/native
Posted by Blair Zajac <bl...@orcaware.com>.
Chia-liang Kao wrote:
> On Sat, Aug 13, 2005 at 08:25:01AM -0400, David James wrote:
>
>>>if (length($buf)) {
>>> return $buf;
>>>} else {
>>> return;
>>>}
>>
>>Good advice, Blair! I think users will benefit from this attention to
>>detail -- I'd recommend this for the 1.3.x line. (I'm not sure what
>>the API compatibility rules are for the Perl bindings, but this change
>>seems like it would be compatible with what users already expect from
>>the Perl bindings API.)
>
>
> I don't think we need to maintain bugward compatibility.
> Blair, would you like to commit this yourself and perhaps
> nominate for 1.2.x along with related changes?
Here's a patch fixing all the return undef's I could find. I don't have the
time to put together a new build to test this, so I'll leave it to you to double
check and do the commit.
BTW, the Perl files have a mixture of tabs and spaces, so the indentation looks
odd in my editor (jove). Our hacking document has a no tab convention, so it
would be good to take a couple of minutes and clean it up.
http://subversion.tigris.org/hacking.html (see Other Conventions)
Regards,
Blair
Re: svn commit: r15700 - trunk/subversion/bindings/swig/perl/native
Posted by Chia-liang Kao <cl...@clkao.org>.
On Sat, Aug 13, 2005 at 08:25:01AM -0400, David James wrote:
> > if (length($buf)) {
> > return $buf;
> > } else {
> > return;
> > }
>
> Good advice, Blair! I think users will benefit from this attention to
> detail -- I'd recommend this for the 1.3.x line. (I'm not sure what
> the API compatibility rules are for the Perl bindings, but this change
> seems like it would be compatible with what users already expect from
> the Perl bindings API.)
I don't think we need to maintain bugward compatibility.
Blair, would you like to commit this yourself and perhaps
nominate for 1.2.x along with related changes?
Cheers,
CLK
PS: good catch!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r15700 - trunk/subversion/bindings/swig/perl/native
Posted by David James <ja...@gmail.com>.
On 8/12/05, Blair Zajac <bl...@orcaware.com> wrote:
> clkao@tigris.org wrote:
> > Author: clkao
> > Date: Fri Aug 12 10:44:46 2005
> > New Revision: 15700
> >
> > Modified:
> > trunk/subversion/bindings/swig/perl/native/Core.pm
> >
> > Log:
> > Follow-up to r15695, more defined-ness tests.
> >
> > * perl/native/Core.pm:
> > (SVN::Core::Stream::READLINE, getline): Fix bugs that content
> > with literal "0" will not be read and returned.
> >
> > Patch by: Pawel Chmielowski <pr...@civ.pl>
> >
> >
> >
> > Modified: trunk/subversion/bindings/swig/perl/native/Core.pm
> > Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/bindings/swig/perl/native/Core.pm?rev=15700&p1=trunk/subversion/bindings/swig/perl/native/Core.pm&p2=trunk/subversion/bindings/swig/perl/native/Core.pm&r1=15699&r2=15700
> > ==============================================================================
> > --- trunk/subversion/bindings/swig/perl/native/Core.pm (original)
> > +++ trunk/subversion/bindings/swig/perl/native/Core.pm Fri Aug 12 10:44:46 2005
> > @@ -201,7 +201,7 @@
> > my $self = shift;
> > *$self->{pool} ||= SVN::Core::pool_create (undef);
> > my ($buf, $eof) = *$self->{svn_stream}->readline ($/, *$self->{pool});
> > - return undef if $eof && !$buf;
> > + return undef if $eof && !length($buf);
>
> As a matter of good Perl style, you don't ever want to return undef. If the
> caller calls the function in an array context, then the function will return an
> array with a single value of undef, which when you test the array for trueness,
> will return true.
>
> So the proper thing to do is just return with no value, which returns undef in
> scalar context and an empty array in array context:
>
> return if $eof && !length($buf);
>
> From perldoc -f return
>
> return EXPR
> return Returns from a subroutine, "eval", or "do FILE" with the value
> given in EXPR. Evaluation of EXPR may be in list, scalar, or
> void context, depending on how the return value will be used,
> and the context may vary from one execution to the next (see
> "wantarray"). If no EXPR is given, returns an empty list in
> list context, the undefined value in scalar context, and (of
> course) nothing at all in a void context.
>
> > return $eof ? $buf : $buf.$/;
> > }
> >
> > @@ -226,7 +226,8 @@
> > return $buf;
> > }
> > elsif (ref $/) {
> > - return *$self->{svn_stream}->read (${$/}) || undef;
> > + my $buf = *$self->{svn_stream}->read (${$/});
> > + return length($buf) ? $buf : undef;
>
> Here you want to do
>
> if (length($buf)) {
> return $buf;
> } else {
> return;
> }
Good advice, Blair! I think users will benefit from this attention to
detail -- I'd recommend this for the 1.3.x line. (I'm not sure what
the API compatibility rules are for the Perl bindings, but this change
seems like it would be compatible with what users already expect from
the Perl bindings API.)
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org