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.
On 14/07/07, chromatic <[EMAIL PROTECTED]> wrote:
On Saturday 14 July 2007 09:05:02 [EMAIL PROTECTED] wrote:
> Author: paultcochrane
> Date: Sat Jul 14 09:05:01 2007
> New Revision: 19863
>
> Modified:
> trunk/lib/Parrot/Configure.pm
> trunk/lib/Parrot/Configure/Data.pm
> trunk/lib/Parrot/Configure/Step.pm
>
> Log:
> [lib] Using explicit returns as per coding standards
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.
BTW: I've committed some other changes about explicit returns before I
saw your email. I'll go through and review those and revert/correct
them as needed.
> 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.
Given that the return statement added above is incorrect, what is the
correct behaviour expected here? Should the routine be returning
C<$self>?
> 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?
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?
> =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.
Paul
