Global Variables in tools/build/ops2pm.pl: What is their rationale? Could they be refactored?
James Keenan
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
Jerry Gay
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