ops2c
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
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
# 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
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
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
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
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
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
Mark: Do you have any objection to closing this RT? Thanks. kid51
[perl #52506] [CAGE] Refactor ops2c
Resolved per discussion with Infinoid.
[perl #52506] [CAGE] Refactor ops2c
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
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
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
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
# 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
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
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
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
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
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
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
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
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
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