Following my refactoring of tools/build/pmc2c.pl, particle asked me to look at phalanxing a couple of the other build tools: ops2pm.pl and ops2c.pl. Since ops2pm.pl is invoked at the very beginning of the 'make' process, I decided to focus there. As was the case with pmc2c.pl, my strategy was to move as much of the code as possible, first into subroutines, then into a module, and then to write a test suite with test scripts that both mimic the workings of ops2pm.pl and try to boost test coverage of statements, branches and conditions.

In the course of this work I've begun to realize that the present of a global variable in the current ops2pm.pl complicates testing. Grepping, then reordering the results in the order in which they would actually be executed, we get:

$ grep -n '$ParrotOps' tools/build/ops2pm.pl
# inside load_op_map_files():
392:    $ParrotOps::max_op_num ||= 0;
407:        if ( exists $ParrotOps::optable{$name} ) {
411:        $ParrotOps::optable{$name} = $number;
412:        if ( $number > $ParrotOps::max_op_num ) {
413:            $ParrotOps::max_op_num = $number;
427:        if ( exists $ParrotOps::optable{$name} ) {
430:        $ParrotOps::skiptable{$name} = 1;

# inside find_op_number():
321:    if ( exists $ParrotOps::optable{$opname} ) {
322:        return $ParrotOps::optable{$opname};
324:    elsif ( exists $ParrotOps::skiptable{$opname} ) {
328: my $n = $ParrotOps::optable{$opname} = ++ $ParrotOps::max_op_num;

# Then ...
189:    my $n      = $ParrotOps::optable{$opname};

So as ops2pm.pl executes, it creates a ParrotOps namespace and, within that namespace, $max_op_num, %optable and %skiptable.

I've always avoided global variables in my own code and have never used them in my test scripts. So it took me some time to realize what pitfalls lay in store as I refactored the code and tried to test it. On the principle of "first, do no harm," I did not and have not tried to eliminate the variables in the ParrotOps namespace. But when I went to write tests for load_op_map_files(), I discovered that if I invoked it twice in the same test file (with relatively minor changes to test some options), I always got failures the second time around -- even though I had isolated the two tests in separate tempdirs. Why? Because the first invocation of the subroutine assigned to the global variables $ParrotOps::max_op_num and $ParrotOps::optable{$name}, so my environment had changed by the point of the second invocation.

Now, I'll grant you that since ops2pm.pl is -- for now -- invoked once and only once during the 'make' process, writiing a test file in which one of its subroutines is invoked twice and being unable to pass on the second invocation is not the end of the world. But I like my tests to be very self-contained and not change (or, more accurately, pollute) their external envionment. And I also like to be able to re-invoke a given subroutine within a given test file as many times as I need to boost its test coverage.

So, as I think about how I might propose to refactor ops2pm.pl, eliminating the global variable comes to mind quickly. But before I do that, I would like to hear comments from other people who have worked on ops2pm.pl over the years it has been evolving into its present form. Perhaps there's some rationale for this on-the-fly creation of the ParrotOps namespace that I'm not aware of.

Feedback, s.v.p.  Thank you very much.

kid51


Reply via email to