ops2c

[Michael Scott]

I'm trying to write some documentation for the ops2c system at the 
moment and have a question.

In Parrot::OpsFile::read_ops() a Parrot::Op's type is set to 'inline' 
or 'function', yet in Parrot::Op type is expected to be 'auto' or 
'manual'.

Auto ops have a 'goto NEXT()' appended to their code.

In src/op.h there is a comment which notes that op type is unused.

Can anyone summarize the history/future of this?

Mike

[perl #52506] [PATCH] Refactor ops2c

[Mark Glines via RT]

Additional testing note.  If you try out this patch, you will need to
remove src/ops/*.c before doing a "make".  Otherwise ops2c won't be
re-run, and you won't actually be testing the patch.

Extra points for anyone who tests it on something other than gcc. :)

Mark

[perl #52506] [PATCH] Refactor ops2c

[via RT]

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


Sekana Fernando reported that src/ops/core_ops.c didn't compile under
g++.  It reports an error about "static op_lib_t core_op_lib" being
declared twice, and rightly so, because it is.  There's an initial stub
declaration, and another declaration at the end of the file with an
initializer block.

It seems appropriate to change the order of things output by ops2c, to
avoid the redundant declaration.  So I did so.  Patch attached.

The ops2c libraries are full of values calculated halfway through the
printing process, within functions named like "print_foo".  And other
"print_bar" functions rely on the values calculated by "print_foo",
which makes it difficult to move around sections of output.  Whenever
I found a value calculated within a print_* function, I just moved it
to the end of new(), instead.  A little MVC goes a long way, I think.

This gets g++ to get past core_ops.c, and on to the next error.  It
passes tests on x86-64 linux, and on ppc darwin.  But I'm hoping for a
little more QA and review, before I check in changes to code this vital.

Mark
Index: lib/Parrot/Ops2c/Utils.pm
===
--- lib/Parrot/Ops2c/Utils.pm   (revision 26793)
+++ lib/Parrot/Ops2c/Utils.pm   (working copy)
@@ -203,6 +203,24 @@
 $argsref->{flag} = $flagref;
 my $self = bless $argsref, $class;
 $self->_iterate_over_ops();
+
+my ( $op_info, $op_func, $getop );
+$op_info = $op_func = 'NULL';
+$getop = '( int (*)(const char *, int) )NULL';
+
+if ($self->{suffix} eq '') {
+$op_func = $self->{bs} . "op_func_table";
+$op_info = $self->{bs} . "op_info_table";
+if (!$self->{flag}->{dynamic}) {
+$getop = 'get_op';
+}
+}
+$self->{getop}   = $getop;
+$self->{op_info} = $op_info;
+$self->{op_func} = $op_func;
+
+$self->{names} = {};
+
 return $self;
 }
 
@@ -439,6 +457,12 @@
 
 $self->_print_preamble_source($SOURCE);
 
+$self->_op_info_table($SOURCE);
+
+$self->_op_func_table($SOURCE);
+
+$self->_print_op_lib_descriptor($SOURCE);
+
 $self->_print_ops_addr_decl($SOURCE);
 
 $self->_print_run_core_func_decl_source($SOURCE);
@@ -460,8 +484,9 @@
 #include "$self->{include}"
 
 $self->{defines}
-static op_lib_t $self->{bs}op_lib;
 
+static int get_op(const char * name, int full);
+
 END_C
 
 my $text = $self->{ops}->preamble( $self->{trans} );
@@ -657,21 +682,11 @@
 
 sub print_c_source_bottom {
 my ( $self, $SOURCE ) = @_;
-my @op_func_table = @{ $self->{op_func_table} };
-my $bs= $self->{bs};
-my $index = $self->{index};
 
 $SOURCE = $self->_reset_line_number($SOURCE);
 
-$self->_op_func_table($SOURCE);
-
-$self->{names} = {};
-$self->_op_info_table($SOURCE);
-
 $self->_op_lookup($SOURCE);
 
-$self->_print_op_lib_descriptor($SOURCE);
-
 $self->_generate_init_func($SOURCE);
 
 $self->_print_dynamic_lib_load($SOURCE);
@@ -706,12 +721,7 @@
 sub _op_func_table {
 my ( $self, $fh ) = @_;
 
-my ( $op_info, $op_func, $getop );
-$op_info = $op_func = 'NULL';
-$getop = '( int (*)(const char *, int) )NULL';
-
 if ( $self->{suffix} eq '' ) {
-$op_func = $self->{bs} . q{op_func_table};
 print $fh <{bs}numops$self->{suffix} = $self->{num_ops};
@@ -720,7 +730,7 @@
 ** Op Function Table:
 */
 
-static op_func$self->{suffix}_t ${op_func}\[$self->{num_entries}] = {
+static op_func$self->{suffix}_t $self->{op_func}\[$self->{num_entries}] = {
 END_C
 
 print $fh @{ $self->{op_func_table} };
@@ -732,9 +742,6 @@
 
 END_C
 }
-$self->{op_info} = $op_info;
-$self->{op_func} = $op_func;
-$self->{getop}   = $getop;
 }
 
 sub _op_info_table {
@@ -749,7 +756,6 @@
 );
 
 if ( $self->{suffix} eq '' ) {
-$self->{op_info} = "$self->{bs}op_info_table";
 
 #
 # Op Info Table:
@@ -826,7 +832,6 @@
 my ( $self, $fh ) = @_;
 
 if ( $self->{suffix} eq '' && !$self->{flag}->{dynamic} ) {
-$self->{getop} = 'get_op';
 my $hash_size = 3041;
 my $tot   = $self->{index} + scalar keys( %{ $self->{names} } );
 if ( $hash_size < $tot * 1.2 ) {
@@ -878,8 +883,6 @@
  * returns >= 0 (found idx into info_table), -1 if not
  */
 
-static int get_op(const char * name, int full);
-
 static size_t hash_str(const char * str) {
 size_t key = 0;
 const char * s;

[perl #52506] [PATCH] Refactor ops2c

[James Keenan via RT]

Following discussion on #parrot with Infinoid, I created the 'ops2c'
branch in our SVN repository to handle refactoring of this area of the
build system.  You can follow developments there with:

svn co https://svn.perl.org/parrot/branches/ops2c

In that branch, I have applied Infinoid's patch to
lib/Parrot/Ops2c/Utils.pm.  On Debian Linux, I have configured, built
and tested through 'make test' and everything is passing.

So, so far, everything is cool.  More to come.

kid51

[perl #52506] [PATCH] Refactor ops2c

[James Keenan via RT]

Following discussion on #parrot with Infinoid, I created the 'ops2c'
branch in our SVN repository to handle refactoring of this area of the
build system.  You can follow developments there with:

svn co https://svn.perl.org/parrot/branches/ops2c

In that branch, I have applied Infinoid's patch to
lib/Parrot/Ops2c/Utils.pm.  On Debian Linux, I have configured, built
and tested through 'make test' and everything is passing.

So, so far, everything is cool.  More to come.

kid51

[perl #52506] [PATCH] Refactor ops2c

[Mark Glines via RT]

On Sun Apr 06 07:39:09 2008, [EMAIL PROTECTED] wrote:
> Following discussion on #parrot with Infinoid, I created the 'ops2c'
> branch in our SVN repository to handle refactoring of this area of the
> build system.  You can follow developments there with:
> 
> svn co https://svn.perl.org/parrot/branches/ops2c
> 
> In that branch, I have applied Infinoid's patch to
> lib/Parrot/Ops2c/Utils.pm.  On Debian Linux, I have configured, built
> and tested through 'make test' and everything is passing.
> 
> So, so far, everything is cool.  More to come.

Ok.  The branch is now at r26827, and I feel like I've run out of
hammers to hit this code with.  Here's what I've done:

* Move all the filehandling/opening/closing/renaming nonsense into a new
function, print_c_source_file().  This complements print_c_header_file,
and removes all the weird filehandle juggling stuff.
* Update t/tools/ops2cutils/ accordingly.
* Reorganize the file so public methods are on top.
* Fix a few things in the POD.

I think a TODO list would help at this point.  Here are some
possibilities I can think of (but don't necessarily feel very strongly
about):

* Honestly, I'm not really sure print_c_source_top and
print_c_source_bottom need to be public methods any more.  In fact, they
could be merged into print_c_source_file entirely.  But separating the
filehandling from the printing is useful for testing... maybe they
should be merged into a print_c_source_contents, or some such.
* We ought to reduce the amount of boilerplate in t/tools/ops2cutils/. 
  I changed the API of 2 functions, and had to update 5 or 6 places in
the test suite.
* Speaking of the test suite, is execution between "Configure.pl" and
"make" really necessary?  Some of the errors I was getting seemed to
indicate it was generating temporary directories, so the documentation
in these tests might be misleading.
* So far I've been ignoring the private methods as much as possible. 
But they could probably use a once-over.
* Even though new() delegates a lot of the work to _iterate_over_ops, it
is still ginormous.  Maybe it should delegate more?  Its length is not
necessarily a bad thing, but it makes it harder for me to understand
what's going on.
* Should probably get someone to test it on win32/msvc.

Is there anything else that's been bugging people about this code?

Mark

[perl #52506] [PATCH] Refactor ops2c

[James Keenan via RT]

As per discussion on #parrot, the remaining TODO questions posed no
obstacle to merging the branch back into trunk.  So I did so in r26830.

[perl #52506] [PATCH] Refactor ops2c

[James Keenan via RT]

On Sun Apr 06 13:13:20 2008, infinoid wrote:
> 
> * Honestly, I'm not really sure print_c_source_top and
> print_c_source_bottom need to be public methods any more.  In fact, they
> could be merged into print_c_source_file entirely.  

True, but ...

> But separating the
> filehandling from the printing is useful for testing... 

Yes, that's still my feeling.  We could make it more elegant but humans
would have a more difficult time figuring out everything that's happening.

> * We ought to reduce the amount of boilerplate in t/tools/ops2cutils/. 

Did some of that in r26882 thru 26884.

kid51

[perl #52506] [CAGE] Refactor ops2c

[James Keenan via RT]

Mark:  Do you have any objection to closing this RT?

Thanks.

kid51

[perl #52506] [CAGE] Refactor ops2c

[James Keenan via RT]

Resolved per discussion with Infinoid.

[perl #52506] [CAGE] Refactor ops2c

[Mark Glines via RT]

On Sat Jun 21 07:33:57 2008, [EMAIL PROTECTED] wrote:
> Mark:  Do you have any objection to closing this RT?

No objection here, my needs regarding g++ builds have been satisfied.

Thanks!

Mark

Re: [perl #52506] [PATCH] Refactor ops2c

[chromatic]

On Saturday 05 April 2008 20:29:45 Mark Glines wrote:

> Sekana Fernando reported that src/ops/core_ops.c didn't compile under
> g++.  It reports an error about "static op_lib_t core_op_lib" being
> declared twice, and rightly so, because it is.  There's an initial stub
> declaration, and another declaration at the end of the file with an
> initializer block.
>
> It seems appropriate to change the order of things output by ops2c, to
> avoid the redundant declaration.  So I did so.  Patch attached.

+1 here

-- c

Re: [perl #52506] [PATCH] Refactor ops2c

[ajr]

On Windows, make, make perl6, and make test all function in ops2c branch
with no more than the customary grumbling.



--

Email and shopping with the feelgood factor!
55% of income to good causes. http://www.ippimail.com

[CVS ci] use ops2c for cgoto, no op_info for prederef

[Leopold Toetsch]

This patch removes a lot of code duplication:
- core_ops_cg.c is built by ops2c.pl now, making ops2cgc.pl obsolete
- core_ops_prederef.c doesn't have an op_info_table anymore, it was just 
a duplication of core_ops.c
- this saves ~300K in sources and makes the interpreter smaller by ~10%
- all core*.c have now an op_lib descriptor

leo

[perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[via RT]

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



r29522 | bernhard | 2008-07-16 11:34:11 -0400 (Wed, 16 Jul 2008) | 2  
lines

Merge the changes from branch 'remove_getfd' back into trunk.

This commit has begun causing failures in the build tools tests.   
Running 'perl Configure.pl --test' today, I got the failures in the  
first attachment:

t/tools/ops2pmutils/08-sort_ops1/? 
#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/08-sort_ops.t line 78.

#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/08-sort_ops.t line 125.
t/tools/ops2pmutils/08-sort_ops33/? 
#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/08-sort_ops.t line 179.

#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/08-sort_ops.t line 313.
# Looks like you failed 4 tests of 86.
t/tools/ops2pmutils/08-sort_ops Dubious, test returned 4 (wstat 
1024, 0x400)
 Failed 4/86 subtests 
t/tools/ops2pmutils/09-prepare_real_ops1/? 
#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/09-prepare_real_ops.t line 71.

#   Failed test 'prepare_real_ops() returned successfully'
#   at t/tools/ops2pmutils/09-prepare_real_ops.t line 73.

#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/09-prepare_real_ops.t line 146.
# Looks like you failed 3 tests of 38.
t/tools/ops2pmutils/09-prepare_real_ops Dubious, test returned 3 (wstat 
768, 0x300)
 Failed 3/38 subtests 
t/tools/ops2pmutils/10-print_module1/? 
#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/10-print_module.t line 76.

#   Failed test 'prepare_real_ops() returned successfully'
#   at t/tools/ops2pmutils/10-print_module.t line 78.

#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/10-print_module.t line 132.

#   Failed test 'prepare_real_ops() returned successfully'
#   at t/tools/ops2pmutils/10-print_module.t line 134.
# Looks like you failed 4 tests of 42.
t/tools/ops2pmutils/10-print_module Dubious, test returned 4 (wstat 
1024, 0x400)
 Failed 4/42 subtests 
t/tools/ops2pmutils/11-print_h.1/? 
#   Failed test 'sort_ops returned successfully'
#   at t/tools/ops2pmutils/11-print_h.t line 74.

#   Failed test 'prepare_real_ops() returned successfully'
#   at t/tools/ops2pmutils/11-print_h.t line 76.
# Looks like you failed 2 tests of 23.
t/tools/ops2pmutils/11-print_h. Dubious, test returned 2 (wstat 
512, 0x200)
 Failed 2/23 subtests 
t/pharness/01-default_testsok 
t/pharness/02-get_test_prog_args...ok 
t/pharness/03-handle_long_options..ok   
t/pharness/04-Usageok   

Test Summary Report
---
t/tools/ops2pmutils/08-sort_ops(Wstat: 1024 Tests: 86 Failed: 4)
  Failed tests:  16, 32, 49, 83
  Non-zero exit status: 4
t/tools/ops2pmutils/09-prepare_real_ops(Wstat: 768 Tests: 38 Failed: 3)
  Failed tests:  16-17, 36
  Non-zero exit status: 3
t/tools/ops2pmutils/10-print_module(Wstat: 1024 Tests: 42 Failed: 4)
  Failed tests:  16-17, 34-35
  Non-zero exit status: 4
t/tools/ops2pmutils/11-print_h (Wstat: 512 Tests: 23 Failed: 2)
  Failed tests:  16-17
  Non-zero exit status: 2
Files=38, Tests=1009, 24 wallclock secs ( 0.16 usr  0.05 sys + 12.72 cusr  0.77 
csys = 13.70 CPU)
Result: FAIL
Failed 4/38 test programs. 13/1009 subtests failed.


Changes made in the 'remove_getfd' branch to lib/Parrot/Ops2c/ 
Utils.pm are the likely source of these failures.  When I reverted to  
the last prior revision of that module, all the build tools tests  
passed.

[li11-226:parrot] 504 $ svn cat -r {2008-07-13} lib/Parrot/Ops2pm/Utils.pm > lib/Parrot/Ops2pm/Utils.pm
[li11-226:parrot] 505 $ make buildtools_tests  
/usr/local/bin/perl t/harness t/tools/pmc2cutils/*.t t/tools/ops2pmutils/*.t t/tools/ops2cutils/*.t t/tools/revision/*.t t/pharness/*.t
t/tools/pmc2cutils/00-qualify..ok 
t/tools/pmc2cutils/01-pmc2cutils...ok
t/tools/pmc2cutils/02-find_fileok   
t/tools/pmc2cutils/03-dump_vtable..ok   
t/tools/pmc2cutils/04-dump_pmc.ok 
t/tools/pmc2cutils/05-gen_cok
t/tools/pmc2cutils/07-open_fileok
t/tools/pmc2cutils/08-pmc-pm...ok   
t/tools/ops2pmutils/00-qualify.ok 
t/tools/ops2pmutils/01-ops2pmutils.

[perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[James Keenan via RT]

On Wed Jul 16 16:56:20 2008, coke wrote:
> > I suspect that the merger/committer failed either to run 'perl
> > Configure.pl --test' or 'make buildtools_tests' prior to 'make'.
> 
> I can't remember the last time I ran these particular test targets, so
> that's easy (for me) to forgive.
> 

(Well, I *could* have written these tests such that they would be
included in 'make test' by default -- but lately it seems people are
more interested in taking tests out of 'make test' than putting them in.
;-) 

The point is that if we make certain tests non-default, then the Parrot
developers who are working on the files which those tests cover must run
the tests prior to commit.)

> [snip]
> 
> The problem appears to be that at some point explicit return
> statements were added here, presumably to help follow that perl critic
> policy. However, they are bare returns, and are not taking advantage
> of the implicit return value that was originally present. (e.g. in
> sort_ops in Parrot/Ops2pm/Utils.pm).

IIRC, in the spring of 2007 my fellow Cage Cleaner went down this
precise road, possibly with this very module.  I.e., he added bare
returns to subs that had quite well-defined final statements.  And he
got the same kind of test failures as we have here.

But when I refactored tools/build/ops2c.pl into
lib/Parrot/Ops2c/Utils.pm earlier that year, it was for the purpose of
refactoring spaghetti code without changing its functionality or calling
into questions the design decisions that, for better or worse, went in
to it.  So that meant that, yes, the final statement in >=1 sub was (a)
a statement that changed the internal state of some variable and (b) had
a return value that, while defined, was not very meaningful.

("Defined, but not very meaningful" -- Does that describe the lives of
anyone you know?)

So if you really think lib/Parrot/Ops2c/Utils.pm could be better
designed while achieving the same functionality, have a go at it!  But
just remember to run the buildtools_tests as you redesign it.  (And run
all the buildtools tests, because Parrot::Ops2pm::Utils depends on
Parrot::Ops2c::Utils.)

Thank you very much.
kid51

[perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[James Keenan via RT]

On Wed Jul 16 18:53:05 2008, [EMAIL PROTECTED] wrote:
> 
> So if you really think lib/Parrot/Ops2c/Utils.pm could be better
> designed while achieving the same functionality, have a go at it!  


But in the short run I would recommend simply reverting to the last
prior version of the module.  I suspect there was nothing in that branch
that really required revisions in Parrot::Ops2c::Utils.

[perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[James Keenan via RT]

On Wed Jul 16 19:25:46 2008, coke wrote:
> 
> Attached is a patch which passes all tests by removing the tests for
> the explicit true value of these methods.
> 

Coke: I don't think we should accept this patch, for the simple reason
that I think the changes made to lib/Parrot/Ops2c/Utils.pm were both
incorrect and unnecessary.  This patch presumes the correctness of those
changes; I dispute that.

In any event, I think we should hear from Barney before proceeding.

Thank you very much.

kid51

[perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[James Keenan via RT]

This module is written in Perl 5 and is called in a program written in
Perl 5.  In the work I've done in this project, I've taken the approach
to return values which I think is more Perlish, namely, if a subroutine
completes and does what you wanted it to, it should return a true value.
 A bare return returns an undef, which in Perl is false.  False values
(whether empty string, empty list, 0 or undef) should indicate some
unsatisfactory completion of the subroutine.

Having these subs end in a bare return to me connotes an unsatisfactory
outcome of some sort.  That's why I did not have them end in bare
returns, and it's why I object to the changes that were made yesterday.

Thank you very much.

kid51

Re: [perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[Will Coleda]

On Wed, Jul 16, 2008 at 7:31 PM, via RT James Keenan
<[EMAIL PROTECTED]> wrote:
> # New Ticket Created by  James Keenan
> # Please include the string:  [perl #57026]
> # in the subject line of all future correspondence about this issue.
> # http://rt.perl.org/rt3/Ticket/Display.html?id=57026 >
>
>
> 
> r29522 | bernhard | 2008-07-16 11:34:11 -0400 (Wed, 16 Jul 2008) | 2
> lines
>
> Merge the changes from branch 'remove_getfd' back into trunk.
>
> This commit has begun causing failures in the build tools tests.
> Running 'perl Configure.pl --test' today, I got the failures in the
> first attachment:
>
>
> t/tools/ops2pmutils/08-sort_ops1/?
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/08-sort_ops.t line 78.
>
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/08-sort_ops.t line 125.
> t/tools/ops2pmutils/08-sort_ops33/?
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/08-sort_ops.t line 179.
>
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/08-sort_ops.t line 313.
> # Looks like you failed 4 tests of 86.
> t/tools/ops2pmutils/08-sort_ops Dubious, test returned 4 
> (wstat 1024, 0x400)
>  Failed 4/86 subtests
> t/tools/ops2pmutils/09-prepare_real_ops1/?
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/09-prepare_real_ops.t line 71.
>
> #   Failed test 'prepare_real_ops() returned successfully'
> #   at t/tools/ops2pmutils/09-prepare_real_ops.t line 73.
>
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/09-prepare_real_ops.t line 146.
> # Looks like you failed 3 tests of 38.
> t/tools/ops2pmutils/09-prepare_real_ops Dubious, test returned 3 
> (wstat 768, 0x300)
>  Failed 3/38 subtests
> t/tools/ops2pmutils/10-print_module1/?
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/10-print_module.t line 76.
>
> #   Failed test 'prepare_real_ops() returned successfully'
> #   at t/tools/ops2pmutils/10-print_module.t line 78.
>
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/10-print_module.t line 132.
>
> #   Failed test 'prepare_real_ops() returned successfully'
> #   at t/tools/ops2pmutils/10-print_module.t line 134.
> # Looks like you failed 4 tests of 42.
> t/tools/ops2pmutils/10-print_module Dubious, test returned 4 
> (wstat 1024, 0x400)
>  Failed 4/42 subtests
> t/tools/ops2pmutils/11-print_h.1/?
> #   Failed test 'sort_ops returned successfully'
> #   at t/tools/ops2pmutils/11-print_h.t line 74.
>
> #   Failed test 'prepare_real_ops() returned successfully'
> #   at t/tools/ops2pmutils/11-print_h.t line 76.
> # Looks like you failed 2 tests of 23.
> t/tools/ops2pmutils/11-print_h. Dubious, test returned 2 
> (wstat 512, 0x200)
>  Failed 2/23 subtests
> t/pharness/01-default_testsok
> t/pharness/02-get_test_prog_args...ok
> t/pharness/03-handle_long_options..ok
> t/pharness/04-Usageok
>
> Test Summary Report
> ---
> t/tools/ops2pmutils/08-sort_ops(Wstat: 1024 Tests: 86 Failed: 4)
>  Failed tests:  16, 32, 49, 83
>  Non-zero exit status: 4
> t/tools/ops2pmutils/09-prepare_real_ops(Wstat: 768 Tests: 38 Failed: 3)
>  Failed tests:  16-17, 36
>  Non-zero exit status: 3
> t/tools/ops2pmutils/10-print_module(Wstat: 1024 Tests: 42 Failed: 4)
>  Failed tests:  16-17, 34-35
>  Non-zero exit status: 4
> t/tools/ops2pmutils/11-print_h (Wstat: 512 Tests: 23 Failed: 2)
>  Failed tests:  16-17
>  Non-zero exit status: 2
> Files=38, Tests=1009, 24 wallclock secs ( 0.16 usr  0.05 sys + 12.72 cusr  
> 0.77 csys = 13.70 CPU)
> Result: FAIL
> Failed 4/38 test programs. 13/1009 subtests failed.
>
>
>
> Changes made in the 'remove_getfd' branch to lib/Parrot/Ops2c/
> Utils.pm are the likely source of these failures.  When I reverted to
> the last prior revision of that module, all the build tools tests
> passed.
>
>
>
>
> I suspect that the merger/committer failed either to run 'perl
> Configure.pl --test' or 'make buildtools_tests' prior to 'make'.

I can't remember the last time I ran these particular test targets, so
that's easy (for me) to for

Re: [perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[Will Coleda]

On Wed, Jul 16, 2008 at 9:53 PM, James Keenan via RT
<[EMAIL PROTECTED]> wrote:
> On Wed Jul 16 16:56:20 2008, coke wrote:
>> > I suspect that the merger/committer failed either to run 'perl
>> > Configure.pl --test' or 'make buildtools_tests' prior to 'make'.
>>
>> I can't remember the last time I ran these particular test targets, so
>> that's easy (for me) to forgive.
>>
>
> (Well, I *could* have written these tests such that they would be
> included in 'make test' by default -- but lately it seems people are
> more interested in taking tests out of 'make test' than putting them in.
> ;-)
>
> The point is that if we make certain tests non-default, then the Parrot
> developers who are working on the files which those tests cover must run
> the tests prior to commit.)
>
>> [snip]
>>
>> The problem appears to be that at some point explicit return
>> statements were added here, presumably to help follow that perl critic
>> policy. However, they are bare returns, and are not taking advantage
>> of the implicit return value that was originally present. (e.g. in
>> sort_ops in Parrot/Ops2pm/Utils.pm).
>
> IIRC, in the spring of 2007 my fellow Cage Cleaner went down this
> precise road, possibly with this very module.  I.e., he added bare
> returns to subs that had quite well-defined final statements.  And he
> got the same kind of test failures as we have here.
>
> But when I refactored tools/build/ops2c.pl into
> lib/Parrot/Ops2c/Utils.pm earlier that year, it was for the purpose of
> refactoring spaghetti code without changing its functionality or calling
> into questions the design decisions that, for better or worse, went in
> to it.  So that meant that, yes, the final statement in >=1 sub was (a)
> a statement that changed the internal state of some variable and (b) had
> a return value that, while defined, was not very meaningful.
>
> ("Defined, but not very meaningful" -- Does that describe the lives of
> anyone you know?)
>
> So if you really think lib/Parrot/Ops2c/Utils.pm could be better
> designed while achieving the same functionality, have a go at it!  But
> just remember to run the buildtools_tests as you redesign it.  (And run
> all the buildtools tests, because Parrot::Ops2pm::Utils depends on
> Parrot::Ops2c::Utils.)
>
> Thank you very much.
> kid51
>

Attached is a patch which passes all tests by removing the tests for
the explicit true value of these methods.

-- 
Will "Coke" Coleda


untest.patch
Description: Binary data

Re: [perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[Will Coleda]

On Wed, Jul 16, 2008 at 10:54 PM, James Keenan via RT
<[EMAIL PROTECTED]> wrote:
> This module is written in Perl 5 and is called in a program written in
> Perl 5.  In the work I've done in this project, I've taken the approach
> to return values which I think is more Perlish, namely, if a subroutine
> completes and does what you wanted it to, it should return a true value.
>  A bare return returns an undef, which in Perl is false.  False values
> (whether empty string, empty list, 0 or undef) should indicate some
> unsatisfactory completion of the subroutine.
>
> Having these subs end in a bare return to me connotes an unsatisfactory
> outcome of some sort.  That's why I did not have them end in bare
> returns, and it's why I object to the changes that were made yesterday.

Not every function has a true/false return value. These methods that
were refactored from the original script are only invoked once, and
are just there to encapsulate code. They are not expected to return a
true -or- false value. The one place they are invoked outside of the
test suite treats them as a void return value and doesn't save the
return value, let alone have a conditional based on it. Since there's
no 'void' return in perl, undef is about as close as you can get. "I'm
done, and I didn't die."

That said, we can agree to disagree on that point for now, and leave
the tests unchanged for the moment...

... because we can change the bare 'return;'s to 'return 1;' for now.
That avoids the implicit return value that we were unintentionally
getting, and will get us one step closer to passing the perlcritic
policy [Subroutines::RequireFinalReturn].

> Thank you very much.
>
> kid51
>



-- 
Will "Coke" Coleda

Re: [perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[chromatic]

On Wednesday 16 July 2008 19:54:57 James Keenan via RT wrote:

> This module is written in Perl 5 and is called in a program written in
> Perl 5.  In the work I've done in this project, I've taken the approach
> to return values which I think is more Perlish, namely, if a subroutine
> completes and does what you wanted it to, it should return a true value.

I don't consider that particularly Perlish; I prefer to throw exceptions on 
failure instead of adding an "Everything's Okay!" alarm to my code.  Yet I 
only bring this up because it's not a practice I've seen consistently applied 
throughout our Perl 5 code.

If it's a practice that enough committers want apply to our code, that's fine 
and I have no strong preference either way -- but it's news to me.

>  A bare return returns an undef, which in Perl is false.

That's not exactly true.  Sometimes it returns an empty list.

> Having these subs end in a bare return to me connotes an unsatisfactory
> outcome of some sort.

Me too, but for a different reason -- it's vestigial magical useless 
copy-and-paste non-functional code added to satisfy a poorly implemented 
Perl::Critic policy.  We should be removing code that doesn't do anything 
useful instead of adding more of it.

-- c

Re: [perl #57026] [BUG]: Changes to Parrot::Ops2c::Utils in remove_getfd branch cause failures in buildtools_tests

[Bernhard Schmalhofer]

Will Coleda schrieb:

On Wed, Jul 16, 2008 at 10:54 PM, James Keenan via RT
<[EMAIL PROTECTED]> wrote:
  

This module is written in Perl 5 and is called in a program written in
Perl 5.  In the work I've done in this project, I've taken the approach
to return values which I think is more Perlish, namely, if a subroutine
completes and does what you wanted it to, it should return a true value.
 A bare return returns an undef, which in Perl is false.  False values
(whether empty string, empty list, 0 or undef) should indicate some
unsatisfactory completion of the subroutine.

Having these subs end in a bare return to me connotes an unsatisfactory
outcome of some sort.  That's why I did not have them end in bare
returns, and it's why I object to the changes that were made yesterday.



Not every function has a true/false return value. These methods that
were refactored from the original script are only invoked once, and
are just there to encapsulate code. They are not expected to return a
true -or- false value. The one place they are invoked outside of the
test suite treats them as a void return value and doesn't save the
return value, let alone have a conditional based on it. Since there's
no 'void' return in perl, undef is about as close as you can get. "I'm
done, and I didn't die."

That said, we can agree to disagree on that point for now, and leave
the tests unchanged for the moment...

... because we can change the bare 'return;'s to 'return 1;' for now.
That avoids the implicit return value that we were unintentionally
getting, and will get us one step closer to passing the perlcritic
policy [Subroutines::RequireFinalReturn].

  

Hi,

sorry for not running 'make buildtool-tests'.

The only reason for adding the empty returns, was that the 
implementation was not in accord with function documentation.

In the POD for the changed functions there was:
Return Value: None.

I think the interface of a function should reflect it's intent. If only 
internal state is modified,
I'm happy with returning undef or the empty list. On the other hand, I 
wouldn't expect that a function returns 1,

only because the code ran successfully.

So I would be in favour of changing the tests.
But as I have no strong feelings about that, a  return 1;  would be fine 
as well.


Regards,
Bernhard