Re: svn commit: r15700 - trunk/subversion/bindings/swig/perl/native

On 8/12/05, Blair Zajac <blair@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 <prefiks@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