[prev in list] [next in list] [prev in thread] [next in thread] 

List:       perl6-internals
Subject:    [perl #43085] [PATCH] Parrot::OpsFile::read_ops():  Can we refactor the setting of VERSION
From:       James Keenan (via RT) <parrotbug-followup () parrotcode ! org>
Date:       2007-05-31 2:30:39
Message-ID: rt-3.6.HEAD-1668-1180578639-1876.43085-72-0 () perl ! org
[Download RAW message or body]

# New Ticket Created by  James Keenan 
# Please include the string:  [perl #43085]
# in the subject line of all future correspondence about this issue. 
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=43085 >


Parrot::OpsFile (lib/Parrot/OpsFile.pm) has, AFAICT, no unit tests  
dedicated to it.  Since it is called at a number of points inside  
Parrot::Ops2c::Utils and Parrot::Ops2pm::Utils (which are in turn  
called by tools/build/ops2c.pl and tools/build/ops2pm.pl, both of  
which are invoked by make), it ends up being well tested by the test  
suites I have written for those two packages (t/tools/ops2cutils/*.t  
and t/tools/ops2pmutils/*.t).

But there is more untested code in this package than I am comfortable  
with.  You can see statement coverage at http://thenceforward.net/ 
parrot/coverage/configure-build/lib-Parrot-OpsFile-pm.html and click  
on links for branch and condition coverage.

Parrot::OpsFile::read_ops() reads an .ops file line-by-line and  
parses it.  Here's some code beginning at line 253.  (I've eliminated  
some whitespace and cuddled some braces to save space.)

             if (m/^\s*VERSION\s*=\s*"(\d+\.\d+\.\d+)"\s*;\s*$/) {
                 if ( exists $self->{VERSION} ) {
                     #die "VERSION MULTIPLY DEFINED!";
                 }
                 $self->version($1);
                 $_ = '';
             }
             elsif (m/^\s*VERSION\s*=\s*PARROT_VERSION\s*;\s*$/) {
                 if ( exists $self->{VERSION} ) {
                     #die "VERSION MULTIPLY DEFINED!";
                 }
                 $self->version( $PConfig{VERSION} );
                 $_ = '';
             }

I interpret this as:  If you come to a line where a 3-part version  
number is assigned to VERSION, capture and use that version number  
(unless for some odd reason the VERSION attribute has already been  
set).  Else, if you come to a line that reads 'VERSION =  
PARROT_VERSION', capture and use that version number.  Else, ...  
well, there's no 'else' stanza here, which is problematic from a  
testing and coverage point of view.

The coverage analysis cited above, however, indicates that the 'if'  
stanza is never reached during testing, i.e., there is no instance of  
something like 'VERSION = 0.4.11;'.  The coverage analysis also  
suggests that the implicit 'else' branch is never reached, either.   
In other words, during testing *only* the 'elsif' branch is reached.

This is not surprising, because if you call 'ack VERSION src/ops/ 
*.ops', you will find 17 instances of 'VERSION = PARROT_VERSION' --  
exactly corresponding to the 17 .ops files in that directory.

	src/ops/bit.ops:5:VERSION = PARROT_VERSION;
	src/ops/cmp.ops:5:VERSION = PARROT_VERSION;
	src/ops/core.ops:9:VERSION = PARROT_VERSION;
	src/ops/debug.ops:5:VERSION = PARROT_VERSION;
	src/ops/experimental.ops:7:VERSION = PARROT_VERSION;
	src/ops/io.ops:5:VERSION = PARROT_VERSION;
	src/ops/math.ops:8:VERSION = PARROT_VERSION;
	src/ops/object.ops:11:VERSION = PARROT_VERSION;
	src/ops/obscure.ops:7:VERSION = PARROT_VERSION;
	src/ops/pic.ops:11:VERSION = PARROT_VERSION;
	src/ops/pmc.ops:10:VERSION = PARROT_VERSION;
	src/ops/set.ops:5:VERSION = PARROT_VERSION;
	src/ops/stack.ops:5:VERSION = PARROT_VERSION;
	src/ops/stm.ops:21:VERSION = PARROT_VERSION;
	src/ops/string.ops:5:VERSION = PARROT_VERSION;
	src/ops/sys.ops:5:VERSION = PARROT_VERSION;
	src/ops/var.ops:5:VERSION = PARROT_VERSION;

In other words, no src/ops/.ops file currently sets VERSION in any  
way other than by assignment from PARROT_VERSION.

I therefore ask:  Why should we not stipulate that 'VERSION =  
PARROT_VERSION' is the *sole* way to set VERSION in an .ops file?  In  
that case, (a) we could eliminate the aforementioned 'if' stanza in  
Parrot::OpsFile::read_ops() and (b) we could probably get away with  
hard-coding 'VERSION = PARROT_VERSION' in exactly one location and  
dispense with coding it in each of the .ops files.

Is there any reason why we should continue to maintain the unused  
option of coding a src/ops/.ops file with a specific 3-part version  
number?  (I'll submit an actual patch if people agree with this.)

Thank you very much.
Jim Keenan

[prev in list] [next in list] [prev in thread] [next in thread]