chromatic wrote:
On Monday 25 December 2006 21:44, James Keenan via RT wrote:
A few style comments here.
Utils.pm
unshift @{$allargsref->{include}}, (
".",
"$FindBin::Bin/../..",
"$FindBin::Bin/../../src/pmc/"
);
Why no File::Spec here?
There are certain ways in which maintenance programming is like open
heart surgery. "First, do no harm" applies with a vengeance. You have
to make sure you don't kill the patient (translation: cause 'make' to
fail), then accomplish your stated objective -- and only then can you
consider fixing what's not fatally broken.
I agree that all that $FindBin::Bin stuff is ugly, but it appears to
have been needed because 'make' changes its working directory a couple
of times and calls pmc2c.pl from at least two different directories. It
was pure hell trying to use it when constructing tests, since my tests
live at a level (t/tools/pmc2cutils) one level deeper than pmc2c.pl.
This meant that I would be forced to decipher error messages with
*three* sets of '../' in them.
In the test suite I deal with this in two ways. In 00-qualify_.t (a
file which is intended to ensure that all the other files can plausibly
work), I have this code:
use Test::More tests => 8;
use FindBin;
use lib (
"$FindBin::Bin/../..",
"$FindBin::Bin/../../lib",
"$FindBin::Bin/../../../lib",
);
use_ok( 'Parrot::Pmc2c::Utils' );
ok(-f "$FindBin::Bin/../../../Makefile", "Makefile located");
ok(-f "$FindBin::Bin/../../../myconfig", "myconfig located");
ok(-f "$FindBin::Bin/../../../lib/Parrot/PMC.pm", "lib/Parrot/PMC.pm
located");
And in all the other test files I have this BEGIN block:
BEGIN {
use FindBin qw($Bin);
use Cwd qw(cwd realpath);
realpath($Bin) =~ m{^(.*\/parrot)\/[^/]*\/[^/]*\/[^/]*$};
$topdir = $1;
if (defined $topdir) {
print "\nOK: Parrot top directory located\n";
} else {
die "Unable to locate top-level Parrot directory";
}
unshift @INC, qq{$topdir/lib};
}
Once I've applied Cwd::realpath() to $Bin, I get a value for $topdir in
which all those ugly '../' segments are cleaned up, which meant my error
messages began to approach intelligibility. The price was that in the
balance of the test files I had to call $main::topdir.
[snip]
if (File::Spec->file_name_is_absolute($file) && -e $file) {
return $file;
}
There's File::Spec here.
=head3 C<dump_vtable()>
$self->dump_vtable("$FindBin::Bin/../../vtable.tbl");
... but not here (this is documentation).
sub dump_vtable {
my $self = shift;
my $file = shift;
Why two shifts here, when @_ goes unused through the rest of the method?
sub print_tree {
my $self = shift;
my $argsref = shift;
Ditto here.
sub find_and_parse_pmc {
my ($self, $file) = @_;
... but not here.
I'll look into the above.
sub gen_parent_list {
my $self = shift;
my ($name, $all) = @_;
This one just confuses me.
Confuses me too. Most of the real action in pmc2c.pl takes place in
subroutines called several levels down inside dump_pmc(). The farther
down you go (a) the less I understand what's going on; (b) the more
difficult is it for me to write tests for what's going on (especially
tests that *only* call an internal subroutine rather than implicitly
testing it via testing its caller); and hence (c) the greater the
likelihood that Devel::Cover is going to report uncovered code. (You
can view coverage reports at:
http://thenceforward.net/parrot/cover_db/coverage.html).
In the case of gen_parent_list(), I never encountered a problem until
9:45 PM last night when -- with all my tests passing -- I went to 'make'
(which had succeeded several times with my new code) and it failed when
it tried to build 'null.dump'. Took me three hours to debug.
Some of the confusion you see arises from the fact that in the current
pmc2c.pl, all of the above are subroutines inside a script. In my
version, pmc2c.pl builds a Parrot::Pmc2c::Utils object and then calls
methods on the object as directed by processing of command-line options.
So much of my early refactoring consisted of eliminating variables
passed to subroutines in favor of extracting that information from the
data structure inside the object. Some methods rely solely on data from
the object itself, while others (including a post-make diagnostic method
like print_tree()) require additional arguments. Other subroutines are
still pure subroutines.
Fertile ground for future refactoring, eh?
kid51
