On Wed Jul 16 16:56:20 2008, coke wrote:
> > I suspect that the merger/committer failed either to run 'perl
> > Configure.pl --test' or 'make buildtools_tests' prior to 'make'.
>
> I can't remember the last time I ran these particular test targets, so
> that's easy (for me) to forgive.
>
(Well, I *could* have written these tests such that they would be
included in 'make test' by default -- but lately it seems people are
more interested in taking tests out of 'make test' than putting them in.
;-)
The point is that if we make certain tests non-default, then the Parrot
developers who are working on the files which those tests cover must run
the tests prior to commit.)
> [snip]
>
> The problem appears to be that at some point explicit return
> statements were added here, presumably to help follow that perl critic
> policy. However, they are bare returns, and are not taking advantage
> of the implicit return value that was originally present. (e.g. in
> sort_ops in Parrot/Ops2pm/Utils.pm).
IIRC, in the spring of 2007 my fellow Cage Cleaner went down this
precise road, possibly with this very module. I.e., he added bare
returns to subs that had quite well-defined final statements. And he
got the same kind of test failures as we have here.
But when I refactored tools/build/ops2c.pl into
lib/Parrot/Ops2c/Utils.pm earlier that year, it was for the purpose of
refactoring spaghetti code without changing its functionality or calling
into questions the design decisions that, for better or worse, went in
to it. So that meant that, yes, the final statement in >=1 sub was (a)
a statement that changed the internal state of some variable and (b) had
a return value that, while defined, was not very meaningful.
("Defined, but not very meaningful" -- Does that describe the lives of
anyone you know?)
So if you really think lib/Parrot/Ops2c/Utils.pm could be better
designed while achieving the same functionality, have a go at it! But
just remember to run the buildtools_tests as you redesign it. (And run
all the buildtools tests, because Parrot::Ops2pm::Utils depends on
Parrot::Ops2c::Utils.)
Thank you very much.
kid51
