On Saturday 14 July 2007 12:57:37 Paul Cochrane wrote:
> I realized a while after having written my last email that I lost a
> bit opportunity in sending such a short reply: to actually *learn*
> something and to improve my skills with Perl. I shall attempt to take
> that opportunity now if you have the time and patience for me.
Sure thing!
> > I don't trust some of these, and it makes me question the utility of this
> > coding standard test.
> I don't think we should blame the test, rather the implementer of the
> changes.
The test promotes adding explicit code to do nothing in certain cases. In my
mind, that's a mistake. It's like the "no declarations in conditionals"
rule. Sometimes that's correct, but the test as written requires us to add
extra code not to do the wrong thing.
> > > Modified: trunk/lib/Parrot/Configure/Data.pm
> > > =======================================================================
> > >==== === --- trunk/lib/Parrot/Configure/Data.pm (original)
> > > +++ trunk/lib/Parrot/Configure/Data.pm Sat Jul 14 09:05:01 2007
> > > @@ -210,7 +210,7 @@
> > > if ( not defined $res ) {
> > > die "You cannot use --step until you have completed the full
> > > configure process\n"; }
> > > - $self->{c}{$_} = $res->{$_}
> > > + return $self->{c}{$_} = $res->{$_}
> > > for CORE::keys %$res;
> > > }
> >
> > This will process the first key (and why CORE::keys?) and then return.
> > That seems to me pretty clearly wrong, or at least a big change in
> > behavior.
>
> My understanding at present is that if a sub doesn't explicitly
> return, then the result of the last statement is returned. Is this
> correct? It seems so in many cases. There are many subroutines which
> I've recently added return statements to where just a single variable
> was present as the last line and it was very obvious that this was
> meant to be returned.
Yes, that's correct.
> Given that the return statement added above is incorrect, what is the
> correct behaviour expected here? Should the routine be returning
> C<$self>?
I don't think it should return anything. The best way to verify that is to
look at all code which calls this sub to see if any of them expect any return
values.
> > > Modified: trunk/lib/Parrot/Configure/Step.pm
> > > =======================================================================
> > >==== === --- trunk/lib/Parrot/Configure/Step.pm (original)
> > > +++ trunk/lib/Parrot/Configure/Step.pm Sat Jul 14 09:05:01 2007
> > > @@ -155,7 +155,7 @@
> > > sub move_if_diff {
> > > my ( $from, $to, $ignore_pattern ) = @_;
> > > copy_if_diff( $from, $to, $ignore_pattern );
> > > - unlink $from;
> > > + return unlink $from;
> > > }
> >
> > I'm not sure the return value of unlink is useful here.
>
> The return value of unlink would get returned here anyway wouldn't it?
Yes.
> In which case, what would be a good return value here? Should I
> check to see if the unlink worked and then return a success or failure
> value?
Again, that depends on if anything actually checked the return value, or if
any of the calling code *should* check the return value.
> > > =item C<genfile($source, $target, %options)>
> > > @@ -403,7 +403,7 @@
> > > close($in) or die "Can't close $source: $!";
> > > close($out) or die "Can't close $target: $!";
> > >
> > > - move_if_diff( "$target.tmp", $target, $options{ignore_pattern} );
> > > + return move_if_diff( "$target.tmp", $target,
> > > $options{ignore_pattern} ); }
> > >
> > > =item C<_run_command($command, $out, $err)>
> > > @@ -472,7 +472,7 @@
> > > sub cc_gen {
> > > my ($source) = @_;
> > >
> > > - genfile( $source, "test.c" );
> > > + return genfile( $source, "test.c" );
> > > }
> > >
> > > =item C<cc_build($cc_args, $link_args)>
> > > @@ -564,7 +564,7 @@
> > > =cut
> > >
> > > sub cc_clean {
> > > - unlink map "test$_", qw( .c .cco .ldo .out), $conf->data->get(qw(
> > > o exe )); + return unlink map "test$_", qw( .c .cco .ldo .out),
> > > $conf->data->get(qw( o exe )); }
> > >
> > > =item C<capture_output($command)>
> >
> > I'm really not sure the return value from unlink is useful here.
>
> Again, what would be a useful return value here? Would returning
> success or failure of the unlink be appropriate?
>
> I'd really appreciate the feedback. I am trying to help out and to
> learn so that (a) I don't make the mistakes again and (b) so that I
> can be more effective in my contributions to the project.
There's really no general rule for what a Perl sub should return, in part
because the language is not purely functional (that is, sometimes you call
subs only because of their side effects) and in part because there's no
enforcement of any particular type or nature of return value on sub
declarations.
The best way to know for sure what we ought to expect is to get a
comprehensive test suite for this code and then make sure it looks sane.
