Closed p5pRT closed 19 years ago
in case your mail browser doesnt inline the 1st attachment\, heres the highlites:
The patch adds 3 things:
1. B::Concise::walk_output($fh)\, with which you can send Concise output to a file\, string (ala: open(my $fh\,'>'\,\$string))\, etc.
2. B::Concise::add_style()\, with which you can add a named style to those already available. This is mostly convenience\, but it lets you register new styles\, then use them like theyre built-in ( including naming them on the command-line\, or as args to B::Concise::compile() )
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework. Tested against bleadperl. all OK.
THIS IS AN ENHANCEMENT PATCH
The patch adds 3 things:
1. B::Concise::walk_output($fh)\, with which you can send Concise output to a file\, string (ala: open(my $fh\,'>'\,\$string))\, etc.
2. B::Concise::add_style()\, with which you can add a named style to those already available. This is mostly convenience\, but it lets you register new styles\, then use them like theyre built-in ( including naming them on the command-line\, or as args to B::Concise::compile()
3. Tests for above. Tested against bleadperl. all OK.
Jim Cromie (via RT) wrote:
# New Ticket Created by Jim Cromie # Please include the string: [perl #24821] # in the subject line of all future correspondence about this issue. # \<URL: http://rt.perl.org/rt3/Ticket/Display.html?id=24821 >
in case your mail browser doesnt inline the 1st attachment\, heres the highlites:
The patch adds 3 things:
1. B::Concise::walk_output($fh)\, with which you can send Concise output to a file\, string (ala: open(my $fh\,'>'\,\$string))\, etc.
2. B::Concise::add_style()\, with which you can add a named style to those already available. This is mostly convenience\, but it lets you register new styles\, then use them like theyre built-in ( including naming them on the command-line\, or as args to B::Concise::compile() )
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework. Tested against bleadperl. all OK
this version supersedes previous\, and adds POD\, and adds a SKIP: block around tests which rely on open(my $fh\, '>'\, \my $scalar) for possible back-porting
Jim Cromie \jcromie@​divsol\.com wrote: :+sub walk_output { :+ $walkHandle = shift; :+ my $iotype = ref $walkHandle; :+ die "expecting GLOB\, FILEHANDLE or IO::* argument" :+ unless $iotype eq 'GLOB' or $iotype =~ /^IO::/; :+}
This sets my alarm bells ringing: if I have a 'Hugo::File' that inherits from an appropriate IO::* module\, this will fail to accept it. I don't think we really have good mechanisms for checking this sort of thing cleanly - perl6 is shaping up to be much better in that respect - but one approach would be to check that $walkHandle->can() each of the methods that you need it to support.
But the patch as a whole looks interesting\, thank you. :)
Hugo
The RT System itself - Status changed from 'new' to 'open'
On Tue\, Jan 06\, 2004 at 02:49:59AM -0000\, Jim Cromie \perlbug\-followup@​perl\.org wrote:
--- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003 +++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004 +# set up 'filehandles' for testing walk_output() +my ($s1\,$s2\,$s3); +open(my $f1\, '>'\, \$s1); +open(my $f2\, '>'\, \$s2); +open(my $f3\, '>'\, \$s3);
Will those opens work without perlio? Do we care?
+my $ref = \<\<'EORef'; +7 \<1> leavesub[1 ref] K/REFC\,1 ->(end) +- \<@> lineseq KP ->7 +1 \<;> nextstate(main -\d+ concise.t:\d+) v ->2 +6 \<2> sassign sKS/2 ->7 +4 \<2> add[t1] sK/2 ->5 +- \<1> ex-rv2sv sK/1 ->3 +2 \<$> gvsv(*b) s ->3 +3 \<$> const(IV 42) s ->4 +- \<1> ex-rv2sv sKRM*/1 ->6 +5 \<$> gvsv(*a) s ->6 +EORef + +# this test is probably somewhat brittle\, despite the qr// +like ($s2\, /\Q$ref/\, "matches against hardcoded reference output");
I think that like() is getting passed "" as the pattern to check\, the *result* of a match against $_; probably not what you intended. Even changed to use qr//\, I don't think you want the \Q\, that will make your \d+ literal. You need to go through and manually backslash all the $*()[].@ chars.
+# try more robust test\, with brittle lib inclusion +SKIP: { + use lib '/usr/local/lib/perl5/site_perl/5.8.2/i686-linux-thread-multi'; + + eval "use String::Approx 'amatch'"; + skip("no String::Approx\, $@"\, 1) if $@; + + my @matches = amatch($s1\, $s2); + + print "@matches\n"; + ok(1\, "I dont have it!"); +}
Not sure exactly what that's for\, but you may want instead: use Config; use lib $Config::Config{sitelib};
lib.pm is supposed to automatically also add the archname subdirectory if it has stuff in it.
Jim Cromie said:
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework.
Thanks very much. Presumably I hadn't added any tests because you hadn't added the framework for them yet. Yes\, that must be it.
-- Paul Johnson - paul@pjcj.net http://www.pjcj.net
Paul Johnson via RT wrote:
Jim Cromie said:
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework.
Thanks very much. Presumably I hadn't added any tests because you hadn't added the framework for them yet. Yes\, that must be it.
huh ?
Ok\, youre Devel::Cover Dude.
I see the following in your pod\, but I dont see how it relates.
Could you elaborate ?
If the module does not use the t/*.t framework:
=head1 ACKNOWLEDGEMENTS
Some code and ideas cribbed from:
Devel::OpProf B::Concise
Yitzchak Scott-Thoennes via RT wrote:
On Tue\, Jan 06\, 2004 at 02:49:59AM -0000\, Jim Cromie \perlbug\-followup@​perl\.org wrote:
--- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003 +++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004 +# set up 'filehandles' for testing walk_output() +my ($s1\,$s2\,$s3); +open(my $f1\, '>'\, \$s1); +open(my $f2\, '>'\, \$s2); +open(my $f3\, '>'\, \$s3);
Will those opens work without perlio? Do we care?
when I ran following\, the above code worked. Should it have failed ?
PERLIO=stdio ./perl -Ilib ext/B/t/concise.t
BTW\, in 2nd version (patch.concise3) I added a SKIP: {eval "use 5.008"; skip if $@\, a bit superfluous for blead\, but...
ALSO\, Ive added $walkHandle->can('print'). to walk_output(). one issue remaining here is whether that should carp\, and if so\, should it preserve the old handle. But thats 'coddling' the user\, and B::Concise was never for 'beginners'
+my $ref = \<\<'EORef'; +7 \<1> leavesub[1 ref] K/REFC\,1 ->(end) +- \<@> lineseq KP ->7 +1 \<;> nextstate(main -\d+ concise.t:\d+) v ->2 +6 \<2> sassign sKS/2 ->7 +4 \<2> add[t1] sK/2 ->5 +- \<1> ex-rv2sv sK/1 ->3 +2 \<$> gvsv(*b) s ->3 +3 \<$> const(IV 42) s ->4 +- \<1> ex-rv2sv sKRM*/1 ->6 +5 \<$> gvsv(*a) s ->6 +EORef + +# this test is probably somewhat brittle\, despite the qr// +like ($s2\, /\Q$ref/\, "matches against hardcoded reference output");
I think that like() is getting passed "" as the pattern to check\, the *result* of a match against $_; probably not what you intended. Even changed to use qr//\, I don't think you want the \Q\, that will make your \d+ literal. You need to go through and manually backslash all the $*()[].@ chars.
Indeed - once I actually added the intended qr//\, test broke. now fixed in my working-copy. Thanks.
+# try more robust test\, with brittle lib inclusion +SKIP: { + use lib '/usr/local/lib/perl5/site_perl/5.8.2/i686-linux-thread-multi'; + + eval "use String::Approx 'amatch'"; + skip("no String::Approx\, $@"\, 1) if $@; + + my @matches = amatch($s1\, $s2); + + print "@matches\n"; + ok(1\, "I dont have it!"); +}
Not sure exactly what that's for\, but you may want instead: use Config; use lib $Config::Config{sitelib};
lib.pm is supposed to automatically also add the archname subdirectory if it has stuff in it.
It was mostly to test another way. Since my 'relative' style added "=> \d+" to the end of each line\, String::Approx would produce a predictable edit-distance between relative and concise outputs.
I gave up when it became obvious that @INC was heavily restricted in core testing\, but left the test below __END__ to elicit feedback. Thanks for 'taking the bait' ;-) I will try your suggestion.
On Wed\, Jan 07\, 2004 at 12:40:41PM -0700\, Jim Cromie wrote:
Paul Johnson via RT wrote:
Jim Cromie said:
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework.
Thanks very much. Presumably I hadn't added any tests because you hadn't added the framework for them yet. Yes\, that must be it.
huh ?
Ok\, youre Devel::Cover Dude.
I see the following in your pod\, but I dont see how it relates. Could you elaborate ?
I wrote the code and docs for using B::Concise outside of the O framework\, but I didn't write any tests\, so I'm very glad that you did.
And yes\, it was to help me writing Devel::Cover.
-- Paul Johnson - paul@pjcj.net http://www.pjcj.net
Jim Cromie wrote:
http://rt.perl.org/rt3/Ticket/Display.html?id=24821 >
The patch adds 3 things:
1. B::Concise::walk_output($fh)\, with which you can send Concise output to a file\, string (ala: open(my $fh\,'>'\,\$string))\, etc.
2. B::Concise::add_style()\, with which you can add a named style to those already available. This is mostly convenience\, but it lets you register new styles\, then use them like theyre built-in ( including naming them on the command-line\, or as args to B::Concise::compile() )
3. Tests for above\, inc test following pod recipe for using B::Concise outside O framework. Tested against bleadperl. all OK
This version supersedes others. I think it adequately addresses all suggestions made here (thanks for those):
1. walk_output() tests GLOB and ->can('print') 2. regex test vs hardcoded string fixed. 3. appears to just work with PERLIO=stdio 4. String::Approx etal tests were cleaned up to actually run\, but are still below __END__ I didnt see enough value in them wrt what wrong distance metric results would mean. the use lib @nearpaths # are a bit hokey
other items 5. added arg-ct tests to set_style\, similar to those in add_style 6. added =head2(s) into pod\, my additions made the subject drift without them. 7. printing to walkHandle in a few more places. 8. no comments yet on the pod\, I'll *assume* thats a good thing.
[jimc@harpo bleadconcise]$ PERLIO=stdio ./perl -Ilib ext/B/t/concise.t 1..30 ok 1 - require B::Concise ok 2 - Smallest OP sequence number ok 3 - Second-smallest OP sequence number ok 4 - Smallest COP sequence number ok 5 - -exec option with //= ok 6 - walk_output() accepts Handle GLOB(0x81478e4) ok 7 - walk_output() accepts Handle GLOB(0x8147920) ok 8 - walk_output() rejects bad Handle ok 9 - walk_output() rejects bad Handle 0 ok 10 - walk_output() rejects bad Handle string ok 11 - walk_output() rejects bad Handle ARRAY(0x81804c4) ok 12 - walk_output() rejects bad Handle HASH(0x822f33c) ok 13 - walk_output() accepts obj that can print ok 14 - add_style detects insufficient args ok 15 - add_style detects insufficient [args] ok 16 - add_style accepts args (stylename => arrayref) ok 17 - add_style correctly disallows re-adding same style-name ok 18 - set_style accepts 3 style-format args ok 19 - set_style rejects bad style-format args ok 20 - use outside O framework ok 21 - open to scalar ok 22 - setup called set_style\, add_style\, add_callback ok 23 - walk_output to opened scalar ok 24 - 3 calls to B::Concise::compile\, wo errs ok 25 - preset style output non-empty ok 26 - concise style output non-empty ok 27 - named style output non-empty ok 28 - preset and named styles are same ok 29 - concise and named styles are different ok 30 - matches against hardcoded reference output
\hv@​crypt\.org writes:
Jim Cromie \jcromie@​divsol\.com wrote: :+sub walk_output { :+ $walkHandle = shift; :+ my $iotype = ref $walkHandle; :+ die "expecting GLOB\, FILEHANDLE or IO::* argument" :+ unless $iotype eq 'GLOB' or $iotype =~ /^IO::/; :+}
This sets my alarm bells ringing: if I have a 'Hugo::File' that inherits from an appropriate IO::* module\, this will fail to accept it. I don't think we really have good mechanisms for checking this sort of thing cleanly - perl6 is shaping up to be much better in that respect - but one approach would be to check that $walkHandle->can() each of the methods that you need it to support.
And another (more OOish) way is not to check\, and just get message like "cannot find method 'write' via Hugo::File" or whatever.
But the patch as a whole looks interesting\, thank you. :)
Hugo
Jim Cromie \jcromie@​divsol\.com writes:
Yitzchak Scott-Thoennes via RT wrote:
On Tue\, Jan 06\, 2004 at 02:49:59AM -0000\, Jim Cromie \perlbug\-followup@​perl\.org wrote:
--- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003 +++ bleadconcise/ext/B/t/concise.t Mon Jan 5 19:20:58 2004 +# set up 'filehandles' for testing walk_output() +my ($s1\,$s2\,$s3); +open(my $f1\, '>'\, \$s1); +open(my $f2\, '>'\, \$s2); +open(my $f3\, '>'\, \$s3);
Will those opens work without perlio? Do we care?
when I ran following\, the above code worked. Should it have failed ?
PERLIO=stdio ./perl -Ilib ext/B/t/concise.t
"Without perlio" means -Uuseperlio as perl build time\, and IO is stdio as in perl \< 5.7.
The \$scalar trick won't work in that case. I don't think we care - beyond skiping the tests if such a perl is used.
BTW\, in 2nd version (patch.concise3) I added a SKIP: {eval "use 5.008"; skip if $@\, a bit superfluous for blead\, but...
ALSO\, Ive added $walkHandle->can('print'). to walk_output(). one issue remaining here is whether that should carp\, and if so\, should it preserve the old handle. But thats 'coddling' the user\, and B::Concise was never for 'beginners'
+my $ref = \<\<'EORef'; +7 \<1> leavesub[1 ref] K/REFC\,1 ->(end) +- \<@> lineseq KP ->7 +1 \<;> nextstate(main -\d+ concise.t:\d+) v ->2 +6 \<2> sassign sKS/2 ->7 +4 \<2> add[t1] sK/2 ->5 +- \<1> ex-rv2sv sK/1 ->3 +2 \<$> gvsv(*b) s ->3 +3 \<$> const(IV 42) s ->4 +- \<1> ex-rv2sv sKRM*/1 ->6 +5 \<$> gvsv(*a) s ->6 +EORef + +# this test is probably somewhat brittle\, despite the qr// +like ($s2\, /\Q$ref/\, "matches against hardcoded reference output");
I think that like() is getting passed "" as the pattern to check\, the *result* of a match against $_; probably not what you intended. Even changed to use qr//\, I don't think you want the \Q\, that will make your \d+ literal. You need to go through and manually backslash all the $*()[].@ chars.
Indeed - once I actually added the intended qr//\, test broke. now fixed in my working-copy. Thanks.
+# try more robust test\, with brittle lib inclusion +SKIP: { + use lib '/usr/local/lib/perl5/site_perl/5.8.2/i686-linux-thread-multi'; + + eval "use String::Approx 'amatch'"; + skip("no String::Approx\, $@"\, 1) if $@; + + my @matches = amatch($s1\, $s2); + + print "@matches\n"; + ok(1\, "I dont have it!"); +}
Not sure exactly what that's for\, but you may want instead: use Config; use lib $Config::Config{sitelib};
lib.pm is supposed to automatically also add the archname subdirectory if it has stuff in it.
It was mostly to test another way. Since my 'relative' style added "=> \d+" to the end of each line\, String::Approx would produce a predictable edit-distance between relative and concise outputs.
I gave up when it became obvious that @INC was heavily restricted in core testing\, but left the test below __END__ to elicit feedback. Thanks for 'taking the bait' ;-) I will try your suggestion.
Nick Ing-Simmons wrote:
\hv@​crypt\.org writes:
Jim Cromie \jcromie@​divsol\.com wrote: :+sub walk_output {
This sets my alarm bells ringing: if I have a 'Hugo::File' that inherits from an appropriate IO::* module\, this will fail to accept it.
And another (more OOish) way is not to check\, and just get message like "cannot find method 'write' via Hugo::File" or whatever.
This Latest revision DOES NOT remove the check\, instead it adds another error-check\, which more explicitly explains the error that confused me initially\, and started me down this pedantic path.
old error:
Can't locate object method "concise" via package "B::NULL"
new error:
[jimc@harpo concise]$ perl outside.pl -exec Not_There main::Not_There: concise_subref(exec\, \&main::Not_There) failed: Possible bad function name (main::Not_There)
[jimc@harpo concise]$ perl outside.pl Not_There
main::Not_There:
concise_subref(basic\, \&main::Not_There) failed: Possible bad function
name (main::Not_There)
The 'problem' with this is that the new die() happens too soon for the actual error to occur\, so I cant be absolutely sure about the cause\, or of the nature of the eventual failure.
I had coded to catch the error\, and then add the "Possible..."\, but I wasnt thrilled with it; it was asymetrical wrt where errors were caught in the 2 modes.
IE - formerly ...
$> perl outside.pl Not_There
main::Not_There: Possible bad function name (main::Not_There) causing (Can't locate object method "concise" via package "B::NULL" at /home/jimc/perl.cpan/concise/B/Concise.pm line 117. ) at /home/jimc/perl.cpan/concise/B/Concise.pm line 231.
The "Possible bad function name" attempts to explain the cause of the error; since B::Concise is a relatively sharp knife\, I chose not to hide the original $@
So\, Im unsure whats the best $@ report\, it currently includes both the outer and inner $@
concise_subref(basic\, \&main::Not_There) failed: $@
2 new tests reflect this new check. ok 30 - '-concise' reports non-existent-function properly ok 31 - '-exec' reports non-existent-function properly
I suppose I could now rip out all the extra die()s\, since theyre not mysterious compared to the 'concise' via B::NULLone\, but your comment sounded more observational than 'concerned'. In any case\, I think that earlier\, more explicit warnings are better\, despite the false comfort that some may infer from them.
ASIDE: that outside.pl script is not part of the patch - I didnt see a place for it\, and it just follows the pod recipe anyway.
thanks.
On Sat\, Jan 10\, 2004 at 04:02:56PM -0700\, Jim Cromie wrote:
Just a couple of apostrophic doc nits:
+B\<add_style> expects args as C\<\< ($styleName => @stylespec) >> or C\<\< +($styleName => \@stylespec) >>\, where @stylespec has 3 strings. It +will also die if you attempt to re-add a known style\, whether its
it's
+standard or previously added by you. + +=head2 add_callback() + +By calling B\<add_callback> and passing references to your callback +subroutines\, you can populate new variables See L\<formatting +specifications>\, or alter the values of existing ones. These +variables are then available for use in the style youve chosen.
you've
-- Paul Johnson - paul@pjcj.net http://www.pjcj.net
"JC" == Jim Cromie \jcromie@​divsol\.com writes:
JC> Nick Ing-Simmons wrote:
NI-S> And another (more OOish) way is not to check\, and just get NI-S> message like "cannot find method 'write' via Hugo::File" or NI-S> whatever.
That's also my usual tendency\, but I'd just chalk it up to implementor laziness (notice a distinct lack of any error checking elsewhere in Concise).
JC> This Latest revision DOES NOT remove the check\, instead it adds JC> another error-check\, which more explicitly explains the error that JC> confused me initially\, and started me down this pedantic path.
JC> old error:
JC> Can't locate object method "concise" via package "B::NULL"
JC> new error:
JC> [jimc@harpo concise]$ perl outside.pl -exec Not_There JC> main::Not_There: JC> concise_subref(exec\, \&main::Not_There) failed: Possible bad function JC> name (main::Not_There)
JC> [...]
It seems to me that the "Possible" is excessively cautious: if the user asks for a function by name\, and that function doesn't exist\, that's the problem\, and we should just give up immediately. I think the check could go before printing the sub name\, for instance.
I've added some more comments on other topics inline in the patch.
JC> diff -ru -x '*.o' bleadperl/ext/B/B/Concise.pm bleadconcise/ext/B/B/Concise.pm JC> --- bleadperl/ext/B/B/Concise.pm Thu Aug 14 02:02:53 2003 JC> +++ bleadconcise/ext/B/B/Concise.pm Sat Jan 10 15:01:21 2004 JC> @@ -14\,10 +14\,11 @@ JC> JC> use Exporter (); # use #5 JC> JC> -our $VERSION = "0.57"; JC> +our $VERSION = "0.58_01";
Please just increment by cents in whatever version eventually gets applied. If Concise gets another 40 patches\, it will be high time to declare 1.0 anyway.
JC> our @ISA = qw(Exporter); JC> our @EXPORT_OK = qw(set_style set_style_standard add_callback JC> - concise_subref concise_cv concise_main); JC> + concise_subref concise_cv concise_main JC> + add_style walk_output); JC> JC> [...] JC> JC> sub B::OP::concise { JC> - my($op\, $level) = @_; JC> + my($op\, $level\, $fh) = @_; JC> + $fh = $walkHandle unless $fh;
Did you add a call to concise with a $fh argument somewhere else I missed? Since many other places use the global $walkHandle unconditionally\, it seems cleaner to just do that here too.
JC> if ($order eq "exec" and $lastnext and $$lastnext != $$op) {
JC> my $h = {"seq" => seq($lastnext)\, "class" => class($lastnext)\,
JC> "addr" => sprintf("%#x"\, $$lastnext)};
JC> - print fmt_line($h\, $gotofmt\, $level+1);
JC> + print $fh fmt_line($h\, $gotofmt\, $level+1);
JC> }
JC> $lastnext = $op->next;
JC> - print concise_op($op\, $level\, $format);
JC> + print $fh concise_op($op\, $level\, $format);
JC> }
JC>
JC> # B::OP::terse (see Terse.pm) now just calls this
JC> @@ -1095\,11 +1125\,16 @@
JC>
JC> =head1 Using B::Concise outside of the O framework
JC>
JC> -It is possible to extend B\<B::Concise> by using it outside of the B\
JC> +
JC> +B\<add_style> expects args as C\<\< ($styleName => @stylespec) >> or C\<\<
JC> +($styleName => \@stylespec) >>\, where @stylespec has 3 strings. It
Could you just pick one of these calling styles? It seems silly to have both.
JC> +will also die if you attempt to re-add a known style\, whether its JC> +standard or previously added by you. JC> + JC> +=head2 add_callback() JC> + JC> +By calling B\<add_callback> and passing references to your callback JC> +subroutines\, you can populate new variables See L\<formatting ^ Punctuation here? --/
JC> +specifications>\, or alter the values of existing ones. These
JC> +variables are then available for use in the style youve chosen.
JC> +
JC> +The subroutines are called in the same order as they are added. Each
JC> +subroutine is passed four parameters. These are a reference to a
JC> +hash\, the keys of which are the names of the variables and the values
JC> +of which are their values\, the op\, the level and the format.
JC>
JC> To define your own variables\, simply add them to the hash\, or change
JC> existing values if you need to. The level and format are passed in as
JC> @@ -1128\,10 +1182\,33 @@
JC> changed or even used.
JC>
JC> To switch back to one of the standard styles like C\
This phrasing seems a bit alarmist to me: all of the things that cause dying are things the programmer should be able to prevent in the course of writing correct code (e.g. passing something that's not a filehandle). How about:
=head2 Errors
None of the above programming interfaces have error codes; they will die() on invalid arguments.
JC> JC> =head1 AUTHOR JC> JC> diff -ru -x '*.o' bleadperl/ext/B/t/concise.t bleadconcise/ext/B/t/concise.t JC> --- bleadperl/ext/B/t/concise.t Tue Feb 4 14:17:24 2003 JC> +++ bleadconcise/ext/B/t/concise.t Sat Jan 10 13:26:31 2004 JC> @@ -6\,7 +6\,7 @@ JC> require './test.pl'; JC> } JC> JC> -plan tests => 5; JC> +plan tests => 31; JC> JC> require_ok("B::Concise"); JC> JC> @@ -35\,3 +35\,260 @@ JC> ); JC> JC> like($out\, qr/print/\, "-exec option with //="); JC> + JC> +######## NEW TESTS ########
Could you think of a more future-proof description than "NEW" to describe the tests that go under this line?
JC> + JC> [...] JC> + JC> +eval { B::Concise::compile('-exec'\,'non_existent_function')->() }; JC> +like ($@\, qr/Possible bad function name \(main::non_existent_function\)/\, JC> + "'-exec' reports non-existent-function properly"); JC> + JC> +__END__ JC> + JC> +# these are interesting\, but they dont add much JC> +# in the way of insight. maybe later..
I would be in favor of physically omitting them. Textual approximate matching doesn't strike me as a good basis for a regression test\, and tests relying on third party modules would also be a lot of trouble.
JC> +SKIP: { ... JC> + eval "use String::Approx 'amatch'"; ... JC> + eval "use Text::Levenshtein 'distance'"; ... JC> +}
-- Stephen
Stephen McCamant wrote:
It seems to me that the "Possible" is excessively cautious:
OK. While cleaning up\, I added the ability to run compile() on \&subname\, which complements the more command-line oriented 'subname'.
With \&sub args\, the $funcname on 1st line of output is replaced by *B::Concise::compile(CODE(......))*\, as I thought a bare 'CODE(0x84ca04c)' would be unhelpful. Im open to either 1 - stripping this 'announcement line'\, 2 - stripping the 0x..... part which makes asub vs asub comparisons more tedious than it needs to be 3 - making this user-settable (kinda overkill)
Also\, I found a bug where callback args were not per docs. #$_->(\%h\, $op\, \$format\, \$level) for @callbacks; $_->(\%h\, $op\, \$level\, \$format\, $stylename) for @callbacks;
Note also that I added another arg to callback invocation. It allows callbacks to act in style specific ways. While this might not be a wise thing for users to do generally\, Perl generally 'gives them enough rope'. I used it in the last test to remove line-number differences from op-nextstate\, which allows me to compare 2 equivalent anonymous subroutines\, and report them as such. More explanation is in the pod in concise.t
I also added B::Concise::_clr_seq()\, which resets the sequence. This is also necessary for the asub vs asub test.
Rest below can be summarized by "OK\, all suggestions accepted\, acted upon"\, along with attached patch.
our $VERSION = "0.58";
JC> sub B::OP::concise { JC> - my($op\, $level) = @_; JC> + my($op\, $level\, $fh) = @_; JC> + $fh = $walkHandle unless $fh;
Did you add a call to concise with a $fh argument somewhere else
Nope. that was 1st whack. now harmonized with other uses.
Could you just pick one of these calling styles? It seems silly to have both.
OK. only C\<\< ($styleName => @stylespec) >> now
^
Punctuation here? --/
Fixed. along with aphostrophe fixes from Paul
JC> +=head1 CAVEATS
now changed to =head2 Errors\, and reworded\, extended
JC> +######## NEW TESTS ########
Could you think of a more future-proof description than "NEW" to describe the tests that go under this line?
:-} now says 0.58 TESTS
JC> +# these are interesting\, but they dont add much JC> +# in the way of insight. maybe later..
I would be in favor of physically omitting them.
Gone. I can play with them elsewhere.
-- Stephen
thnaks --jimc
folks\,
I picked this back up recently\, I think it now incorporates all comments from this thread started 1/6/04\, plus a few off-list suggestions.
In the interest of full disclosure\, theres (at least) a couple of maybes in here:
1. some regexp match failures under debugger (only).
these may expose a bug\, and also suggests that stringifying a structure (an optree in this case) then string-matching it against something else is not robust generally. In this case\, I hope its enough.
$> ./perl -d -Ilib ext/B/t/concise.t
... not ok 43 - 2stringout vs hardcoded reference data # Failed at ext/B/t/concise.t line 394 # got 'B::Concise::compile(CODE(0x85c98ac)) # 7 \<1> leavesub[2 refs] K/REFC\,1 ->(end) # - \<@> lineseq KP ->7 # 1 \<;> dbstate(main -803 concise.t:389) v ->2 # 6 \<2> sassign sKS/2 ->7 # 4 \<2> add[t3] sK/2 ->5 # - \<1> ex-rv2sv sK/1 ->3 # 2 \<#> gvsv[*b] s ->3 # 3 \<$> const[IV 42] s ->4 # - \<1> ex-rv2sv sKRM*/1 ->6 # 5 \<#> gvsv[*a] s ->6 # ' # expected /(?-xism:^B::Concise::compile\(CODE\(0x[0-9a-fA-F]+\)\) # 7 \<1> leavesub\[1 ref\] K/REFC\,1 ->\(end\) # - \\\@​ lineseq KP ->7 # 1 \<;> nextstate\(main -\d+ concise\.t:\d+\) v ->2 # 6 \<2> sassign sKS/2 ->7 # 4 \<2> add\[t3\] sK/2 ->5 # - \<1> ex-rv2sv sK/1 ->3 # 2 \<\#> gvsv\[\*b\] s ->3 # 3 \<\$> const\[IV 42\] s ->4 # - \<1> ex-rv2sv sKRM\*/1 ->6 # 5 \<\#> gvsv\[\*a\] s ->6 # $)/
2. Above output shows how my 'announcement' line displays an anonymous sub. its close to this analogous form (from bleadperl\, unmodified)
[jimc@harpo bleadperl]$ perl -MO=Concise\,foo -e 'sub foo {1}; foo' main::foo: 3 \<1> leavesub[1 ref] K/REFC\,1 ->(end) - \<@> lineseq KP ->3 1 \<;> nextstate(main 1 -e:1) v ->2 2 \<$> const[IV 1] s ->3 -e syntax OK
whereas bleadperl doesnt produce an announcement line for an anonymous sub.
[jimc@harpo bleadperl]$ perl -MO=Concise -e 'sub {1}' 6 \<@> leave[1 ref] vKP/REFC ->(end) 1 \<0> enter ->2 2 \<;> nextstate(main 2 -e:1) v ->3 5 \<1> refgen vK/1 ->6 - \<1> ex-list lKRM ->5 3 \<0> pushmark sRM ->4 4 \<$> anoncode[CV ] lRM ->5 -e syntax OK
the question here is - whether I should revert the asub announce\, and if not\, should I simplify it - get rid of () parens. A space would be visually less cluttered. and/or get rid of the stringified "$coderef". I put it there to disambiguate between 2 reports.
3. 'extra' arg if invoked with extra arg\, last test prints output directly. I stuffed this in cuz I didnt grok why the 'goto -' lines were printed in -exec mode and I wanted to flag it to y'all.
I think its cuz of a flawed set of style-formats\, or cuz of a prob with my callback\, and *not* cuz of a bug I added to B::Concise.
That said\, I did add a 5th arg to the callback\, namely the current $stylename. This allows any callback to do style-specific stuff\, albeit in a slightly hackish way. It seemed overkill/incompatible to rework the callback dispatch itself to pay heed to the stylename. So I chose to 'give em the rope'\, maybe theyll pull a car out of a ditch.
$> ./perl -Ilib ext/B/t/concise.t Foo
ok 46 - suppressed all but 3 lines of output con-scope: B::Concise::compile(CODE(0x81b5088)) 7 \<1> leavesub[1 ref] K/REFC\,1 ->(end) 1 \<;> nextstate(main -464 concise.t:455) v ->2
exec-scope: B::Concise::compile(CODE(0x8275d84)) goto - 1 \<;> nextstate(main -463 concise.t:464) v 7 \<1> leavesub[1 ref] K/REFC\,1 B::Concise::compile(CODE(0x8283fac)) goto - 8 \<;> nextstate(main -462 concise.t:465) v 9 \<#> gvsv[*b] s a \<$> const[IV 42] s b \<2> add[t3] sK/2 c \<#> gvsv[*a] s d \<2> sassign sKS/2 e \<1> leavesub[1 ref] K/REFC\,1
jim cromie wrote:
I picked this back up recently\, I think it now incorporates all comments from this thread started 1/6/04\, plus a few off-list suggestions.
Thanks\, I applied the Concise.pm part of your patch as change #22539.
I didn't apply the test patch\, because : 1. you probably missed the fact that Concise returns different output with threaded and unthreaded perls 2. you gave me an idea for a Grand Plan.
The other day I added an optimisation that basically transforms my $x = undef; into my $x; but I didn't add a test for it\, because I didn't knew how to do it. Thus\, if we then modify the optree building in a way that breaks this optimisation\, this will go unnoticed\, and this is not a good thing. But your patch to B::Concise gives a tool to test such things.
So what I'd like is a new test file\, let's say ext/B/t/optrees.t\, that lists code snippets and expected optrees in a way perhaps inspired by the things under t/lib/warnings/*\, in a way that it's easy to add new stuff ; that takes into account perl configuration differences (useithreads for example) ; and in which in the future we would add regression tests for optree optimisations. (@x = sort @x in place comes to mind as well.) What do you think about this ?
Rafael Garcia-Suarez wrote:
jim cromie wrote:
I picked this back up recently\, I think it now incorporates all comments from this thread started 1/6/04\, plus a few off-list suggestions.
Thanks\, I applied the Concise.pm part of your patch as change #22539.
:-)
I didn't apply the test patch\, because : 1. you probably missed the fact that Concise returns different output with threaded and unthreaded perls
true - I never thought to test that. I can repackage tests 1-20\, which dont actually run compile\, and so wouldnt see any threading diffs. I could also try to rewrite 21-46 to be thread-independent\, it may be as simple as having 2 versions of ref-text.
BTW - if you still have the broken non-threaded test output\, pls send to me. Ill have to build a non-threaded blead anyway\, but it will help dispell my fear that concise-nothreads.t & concise-threads.t might be needed for 21-46
2. you gave me an idea for a Grand Plan.
The other day I added an optimisation that basically transforms my $x = undef; into my $x; but I didn't add a test for it\, because I didn't knew how to do it. Thus\, if we then modify the optree building in a way that breaks this optimisation\, this will go unnoticed\, and this is not a good thing. But your patch to B::Concise gives a tool to test such things.
Hey look - brittleness IS a feature ;-) For this\, I suppose that its better to have false positives than false negatives. But actually\, this test is actually more robust than comparing to a ref-string.
is ($s1\, $s2\, "assign properly optimized away")
BTW - theres another optimization yet to do. I'll see if I can follow your optimization patch.
[jimc@harpo bleadperl]$ ./perl -MO=Concise -e 'my $foo=()'; 6 \<@> leave[1 ref] vKP/REFC ->(end) 1 \<0> enter ->2 2 \<;> nextstate(main 1 -e:1) v ->3 5 \<2> sassign vKS/2 ->6 3 \<0> stub sP ->4 4 \<0> padsv[$foo:1\,2] sRM*/LVINTRO ->5 -e syntax OK
our $foo=undef is also optimizable\, though probly not worth it. (approx once per script\, highly unlikely inside a subroutine -might even warrant a warning\, under use warnings 'paranoid')
So what I'd like is a new test file\, let's say ext/B/t/optrees.t\, that lists code snippets and expected optrees in a way perhaps inspired by the things under t/lib/warnings/*\, in a way that it's easy to add new stuff ; that takes into account perl configuration differences (useithreads for example) ; and in which in the future we would add regression tests for optree optimisations. (@x = sort @x in place comes to mind as well.) What do you think about this ?
hmm - while t/lib/warnings/* looks somewhat daunting\, is this a crude but reasonable starting point ?
ext/B/t/optweaks/{assign\,sort\,map-scalar\,grep-boolean}.t
thx\, jimc
On Sat\, Mar 20\, 2004 at 12:36:56AM +0100\, Rafael Garcia-Suarez \rgarciasuarez@​free\.fr wrote:
jim cromie wrote:
I picked this back up recently\, I think it now incorporates all comments from this thread started 1/6/04\, plus a few off-list suggestions.
Thanks\, I applied the Concise.pm part of your patch as change #22539.
I didn't apply the test patch\, because : 1. you probably missed the fact that Concise returns different output with threaded and unthreaded perls 2. you gave me an idea for a Grand Plan.
The other day I added an optimisation that basically transforms my $x = undef; into my $x; but I didn't add a test for it\, because I didn't knew how to do it. Thus\, if we then modify the optree building in a way that breaks this optimisation\, this will go unnoticed\, and this is not a good thing. But your patch to B::Concise gives a tool to test such things.
So what I'd like is a new test file\, let's say ext/B/t/optrees.t\, that lists code snippets and expected optrees in a way perhaps inspired by the things under t/lib/warnings/*\, in a way that it's easy to add new stuff ; that takes into account perl configuration differences (useithreads for example) ; and in which in the future we would add regression tests for optree optimisations. (@x = sort @x in place comes to mind as well.) What do you think about this ?
If you look at op/split.t\, there's a test there that "my ($a\,$b) = split;" uses a default limit of 3. It might be better to add additional capability to test.pl to make that kind of testing easier.
@tux - Status changed from 'open' to 'resolved'
Migrated from rt.perl.org#24821 (status was 'resolved')
Searchable as RT24821$