Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reimplement List.min in Perl6 instead of PIR #754

Closed
p6rt opened this issue Mar 8, 2009 · 11 comments
Closed

Reimplement List.min in Perl6 instead of PIR #754

p6rt opened this issue Mar 8, 2009 · 11 comments
Labels

Comments

@p6rt
Copy link

p6rt commented Mar 8, 2009

Migrated from rt.perl.org#63712 (status was 'resolved')

Searchable as RT63712$

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

From @bacek


src/builtins/any-list.pir | 47 ---------------------------------------------
src/setting/Any-list.pm | 20 ++++++++++++++++++-
2 files changed, 19 insertions(+), 48 deletions(-)

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

From @bacek

059ec2c89dc266165efcee344f248218be567fe4.diff
diff --git a/src/builtins/any-list.pir b/src/builtins/any-list.pir
index b87666b..bc5cc9e 100644
--- a/src/builtins/any-list.pir
+++ b/src/builtins/any-list.pir
@@ -223,53 +223,6 @@ Return a List with the keys of the invocant.
     .return(res)
 .end
 
-=item min
-
-=cut
-
-.namespace []
-.sub 'min' :multi()
-    .param pmc values          :slurpy
-    .local pmc by
-    by = get_hll_global 'infix:cmp'
-    unless values goto have_by
-    $P0 = values[0]
-    $I0 = isa $P0, 'Sub'
-    unless $I0 goto have_by
-    by = shift values
-  have_by:
-    .tailcall values.'min'(by)
-.end
-
-
-.namespace ['Any']
-.sub 'min' :method :multi(_)
-    .param pmc by              :optional
-    .param int has_by          :opt_flag
-    if has_by goto have_by
-    by = get_hll_global 'infix:cmp'
-  have_by:
-
-    .local pmc it, result
-    $P0 = self.'list'()
-    it = $P0.'iterator'()
-    unless it goto fail
-    result = shift it
-  loop:
-    unless it goto done
-    $P0 = shift it
-    $I0 = by($P0, result)
-    unless $I0 < 0 goto loop
-    result = $P0
-    goto loop
-  fail:
-    .local num failres
-    failres = "+Inf"
-    .return (failres)
-  done:
-    .return (result)
-.end
-
 
 .namespace []
 .sub 'max' :multi()
diff --git a/src/setting/Any-list.pm b/src/setting/Any-list.pm
index 5758dce..0aaeccc 100644
--- a/src/setting/Any-list.pm
+++ b/src/setting/Any-list.pm
@@ -23,7 +23,20 @@ class Any is also {
             $res = &$expression($res, |@args);
         }
         $res;
-    }
+    };
+
+    # RT #63700 - parse failed on &infix:<cmp>
+    our Array multi method min( $values: Code $by = sub { $^a cmp $^b } ) {
+        my @list = $values.list;
+        return +Inf unless @list.elems;
+        my $res = @list.shift;
+        for @list -> $x {
+            if (&$by($res, $x) > 0) {
+                $res = $x;
+            }
+        }
+        $res;
+    };
 }
 
 our List multi grep(Code $test, *@values) {
@@ -34,4 +47,9 @@ multi reduce ( Code $expression ;; *@values ) {
     @values.reduce($expression);
 }
 
+our List multi min(*@values) {
+    my $by = @values[0] ~~ Code ?? shift @values !! sub { $^a cmp $^b };
+    @values.min($by);
+}
+
 # vim: ft=perl6

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

From @moritz

On Sat Mar 07 21​:50​:00 2009, bacek wrote​:

---
src/builtins/any-list.pir | 47
---------------------------------------------
src/setting/Any-list.pm | 20 ++++++++++++++++++-
2 files changed, 19 insertions(+), 48 deletions(-)

Applied as a7214ac28c5c7c47932f1e76a15c8707524f964d, thanks.

Moritz

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

@moritz - Status changed from 'open' to 'resolved'

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

@pmichaud - Status changed from 'resolved' to 'open'

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

From @pmichaud

On Sat, Mar 07, 2009 at 09​:50​:02PM -0800, Vasily Chekalkin wrote​:

+our List multi min(*@​values) {
+ my $by = @​values[0] ~~ Code ?? shift @​values !! sub { $^a cmp $^b };
+ @​values.min($by);
+}

This doesn't match the spec -- the $by parameter is required.
At any rate, the first argument should not be explicitly checked
for being a Code object -- if we do this it should be via a multi
signature.

Yes, the PIR code was "cheating" in this respect, but I don't
want to blindly copy our cheats from PIR into the setting without
subjecting them to another review.

+ # RT #​63700 - parse failed on &infix​:<cmp>
+ our Array multi method min( $values​: Code $by = sub { $^a cmp $^b } ) {
+ my @​list = $values.list;
+ return +Inf unless @​list.elems;
+ my $res = @​list.shift;
+ for @​list -> $x {
+ if (&$by($res, $x) > 0) {
+ $res = $x;
+ }
+ }
+ $res;
+ };
}

Why are C<min> and C<max> specced as returning C<Array> and C<List>?
Shouldn't they just return the single element that is the minimum
or maximum from the list?

Pm

@p6rt
Copy link
Author

p6rt commented Mar 8, 2009

From @moritz

Patrick R. Michaud wrote​:

On Sat, Mar 07, 2009 at 09​:50​:02PM -0800, Vasily Chekalkin wrote​:

+our List multi min(*@​values) {
+ my $by = @​values[0] ~~ Code ?? shift @​values !! sub { $^a cmp $^b };
+ @​values.min($by);
+}

This doesn't match the spec -- the $by parameter is required.
At any rate, the first argument should not be explicitly checked
for being a Code object -- if we do this it should be via a multi
signature.

I've fixed that, and corrected the spec tests accordingly.

Yes, the PIR code was "cheating" in this respect, but I don't
want to blindly copy our cheats from PIR into the setting without
subjecting them to another review.

+ # RT #​63700 - parse failed on &infix​:<cmp>
+ our Array multi method min( $values​: Code $by = sub { $^a cmp $^b } ) {
+ my @​list = $values.list;
+ return +Inf unless @​list.elems;
+ my $res = @​list.shift;
+ for @​list -> $x {
+ if (&$by($res, $x) > 0) {
+ $res = $x;
+ }
+ }
+ $res;
+ };
}

Why are C<min> and C<max> specced as returning C<Array> and C<List>?
Shouldn't they just return the single element that is the minimum
or maximum from the list?

I agree, so I removed the non-sense return types from the specs.

Cheers,
Moritz

@p6rt
Copy link
Author

p6rt commented Mar 9, 2009

From @bacek

Patrick R. Michaud via RT wrote​:

On Sat, Mar 07, 2009 at 09​:50​:02PM -0800, Vasily Chekalkin wrote​:

+our List multi min(*@​values) {
+ my $by = @​values[0] ~~ Code ?? shift @​values !! sub { $^a cmp $^b };
+ @​values.min($by);
+}

This doesn't match the spec -- the $by parameter is required.
At any rate, the first argument should not be explicitly checked
for being a Code object -- if we do this it should be via a multi
signature.

Yes, the PIR code was "cheating" in this respect, but I don't
want to blindly copy our cheats from PIR into the setting without
subjecting them to another review.

This is workaround for this situation​:

<bacek> rakudo​: multi sub foo(Code &x, *@​values) {...}; multi sub
foo(*@​values) {...}; say foo(sub {}, 1,2,3);
<p6eval> rakudo 2daf6b​: OUTPUT«Ambiguous dispatch to multi 'foo'.
Ambiguous candidates had signatures​:␤​:(Code x, Any @​values)␤​:(Any
@​values)␤␤current instr.​: '_block14' pc 101 (EVAL_18​:52)␤»

Should I fill bug report for this?

--
Bacek

@p6rt
Copy link
Author

p6rt commented Mar 10, 2009

From @pmichaud

On Mon Mar 09 00​:41​:24 2009, bacek wrote​:

Patrick R. Michaud via RT wrote​:

At any rate, the first argument should not be explicitly checked
for being a Code object -- if we do this it should be via a multi
signature.

Yes, the PIR code was "cheating" in this respect, but I don't
want to blindly copy our cheats from PIR into the setting without
subjecting them to another review.

This is workaround for this situation​:

<bacek> rakudo​: multi sub foo(Code &x, *@​values) {...}; multi sub
foo(*@​values) {...}; say foo(sub {}, 1,2,3);
<p6eval> rakudo 2daf6b​: OUTPUT«Ambiguous dispatch to multi 'foo'.
Ambiguous candidates had signatures​:␤​:(Code x, Any @​values)␤​:(Any
@​values)␤␤current instr.​: '_block14' pc 101 (EVAL_18​:52)␤»

Should I fill bug report for this?

Yes please. Closing this ticket, thanks!

Pm

@p6rt
Copy link
Author

p6rt commented Mar 10, 2009

@pmichaud - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this as completed Mar 10, 2009
@p6rt p6rt added the patch label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant