On Sat, Jul 14, 2007 at 04:02:50PM -0400, James E Keenan wrote:
> chromatic 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 agree. I haven't done a thorough check, but I've already spotted at
> least one case, in lib/Parrot/Configure.pm, where addition of 'return'
> changed the behavior of the function.
>
> ###################
>
> sub runstep {
> my $self = shift;
> my $taskname = shift;
>
> my ( $verbose, $verbose_step, $ask ) =
> $self->options->get( qw( verbose verbose-step ask ) );
>
> for my $task ( $self->steps() ) {
> if ( $task->{"Parrot::Configure::Task::step"} eq $taskname ) {
> $self->_run_this_step( {
> task => $task,
> verbose => $verbose,
> verbose_step => $verbose_step,
> ask => $ask,
> n => 1,
> } );
> }
> }
> return; # <-- recently added -- and erroneous
> }
>
> ###################
>
> Until the addition of that 'return' statement, the return value of the
> method would have been the truth value of the last statement evaluated
> within the method -- most likely, the evaluation of the last iteration
> of _run_this_step, but (if the string equality were not true) possibly
> the evaluation of the get() call earlier on. In either case, the
> evaluation of the last statement and, hence, the return value of the
> subroutine was overwhelmingly likely to be a true value.
Or alternatively the value of something in the for() statement. IIRC it's
undefined (or at least not yet defined) whether a control statements such
as for() and if() count as 'last statement evaluated' for this purpose.
I'd definitely support adding an explicit return in this function. Just
ensure that it gets the right value as its argument.
Nicholas Clark
