On 1/12/07, James Keenan <[EMAIL PROTECTED]> wrote:
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.
some time ago, when these scripts were written, there was no policy to use strict or warnings in perl scripts. at some point, strict and warnings pragmas were enabled in all perl sources. this was a large conversion, and without proper tests, it was impossible to determine, except by running the tools, whether or not they worked as designed. it seems that adding 'my' to the first mention of the variable in question was enough to keep strict happy, and nobody noticed the script behaving differently. but, as you point out, global variables are usually improper, and i believe that in this case the loop variable should have it's own scope. feel free to refactor it in order to make it more testable. ~jerry
