On Wed, Jul 16, 2008 at 9:53 PM, James Keenan via RT
<[EMAIL PROTECTED]> wrote:
> 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
>Attached is a patch which passes all tests by removing the tests for the explicit true value of these methods. -- Will "Coke" Coleda
untest.patch
Description: Binary data
