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

Fix Parcel.sort (fixes the very first example in http://cloud.github.com/downloads/perl6/book/book-2010-04.pdf) and Hash.sort #1686

Closed
p6rt opened this issue Apr 13, 2010 · 8 comments
Labels

Comments

@p6rt
Copy link

p6rt commented Apr 13, 2010

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

Searchable as RT74334$

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From quester.pm@gmail.com

Hello,

I noticed that certain uses of sort no longer work in Rakudo after the ng
merge in February 2010. For example, this line in the table tennis example
in the Perl 6 book...

  my @​sorted = @​names.sort({ %sets{$_} }).sort({ %games{$_} }).reverse

fails, as does this trivial example,

  $ rakudo -e '<1 3 4 2>.sort({-$^a}).say'
  Too many positional parameters passed; got 2 but expected 1...

The attached patch fixes the problem as follows​:

1. In Seq.pm, the sort method is modified to make a ResizablePMCArray to
use with Parrot's sort method. The original code said that it did so in the
comments, but it actually made a Parcel instead. This first patch doesn't
fix anything by itself, but without it, the change to Parcel.pm would cause
an infinite loop.
2. In Parcel.pm, a sort method is added, which uses the sort in Seq.pm.
This fixes both of the examples above.
3. Hash.pm is also modified to use the sort in Seq.pm. This allows the
"%hash.sort returns a List of Pairs" test in t/spec/S32-list/sort.t to work.
4. Subroutine versions of sort are provided in Seq.pm and Hash.pm. (But
not in Parcel.pm.... the distinction between Parcel and Seq is lost in
subroutine calls. The subroutines in Seq.pm work for both parcel and array
parameters.)

Note that one test in t/spec/S32-list/sort.t fails...

  not ok 4 - array of mixed numbers including Inf/NaN
  # got​: [-Inf, -61/20, -1e-07, 1/10, 11/10, 2, 42, Inf, NaN]
  # expected​: [NaN, -Inf, -61/20, -1e-07, 1/10, 11/10, 2, 42, Inf]

This is due to NaN sorting greater than Inf, rather than less than -Inf.
Since this is consistent with the Rakudo spaceship operator...

  $ perl6 -e 'say NaN <=> Inf'
  1

... I'm not sure that is appropriate to "fix" it in the code; it might be
better to change the test to match Rakudo's behavoir, or perhaps the test
should accept either >+Inf or <-Inf. The IEEE 754 standard just specifies
that than NaN should be "unordered".

Please let me know if you have any comments or concerns or would rather see
a different approach.

Best regards,
Ira Kevin Byerly
quester (on #perl6 and perlmonks)
quester.pm@​gmail.com

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From quester.pm@gmail.com

0001-Fix-Parcel.sort-fixes-the-very-first-example-in-http.patch
From 37d13f5c76c35d03a69b6ff27d9f29b7699bbef0 Mon Sep 17 00:00:00 2001
From: quester <quester.pm@gmail.com>
Date: Tue, 13 Apr 2010 00:03:45 -1000
Subject: [PATCH] Fix Parcel.sort (fixes the very first example in http://cloud.github.com/downloads/perl6/book/book-2010-04.pdf) and Hash.sort

---
 src/core/Hash.pm   |    9 +++++++++
 src/core/Parcel.pm |    2 ++
 src/core/Seq.pm    |   10 ++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/core/Hash.pm b/src/core/Hash.pm
index 29f6f4c..31ca6aa 100644
--- a/src/core/Hash.pm
+++ b/src/core/Hash.pm
@@ -101,4 +101,13 @@ role Hash is EnumMap {
             self.{$key} = $value;
         }
     }
+
+    multi method sort(&by = &infix:<cmp>) {
+        self.pairs.sort(&by)
+    }
+
 }
+
+multi sub sort (%h, :&by = &infix:<cmp>) { %h.pairs.sort(&by) }
+multi sub sort (&by, %h) { %h.pairs.sort(&by) }
+
diff --git a/src/core/Parcel.pm b/src/core/Parcel.pm
index 22014b0..3f060ac 100644
--- a/src/core/Parcel.pm
+++ b/src/core/Parcel.pm
@@ -5,6 +5,8 @@ augment class Parcel {
 
     method rotate(Int $n = 1) { self.Seq.rotate($n) }
 
+    multi method sort(&by = &infix:<cmp>) { self.Seq.sort(&by) }
+
     multi method ACCEPTS($x) {
         # smart-matching against Nil
         if self.elems == 0 {
diff --git a/src/core/Seq.pm b/src/core/Seq.pm
index 3b14404..cac25c2 100644
--- a/src/core/Seq.pm
+++ b/src/core/Seq.pm
@@ -61,17 +61,20 @@ augment class Seq {
         # of indices (from 0 to $list.elems), then use that RPA
         # as a slice into self.
 
+        my $index_PARROT_RPA = pir::new__PS("ResizablePMCArray");
+        pir::push__vPP($index_PARROT_RPA, $_) for ^self.elems; 
+
         # If &by.arity < 2, then it represents a block to be applied
         # to the elements to obtain the values for sorting.
         if (&by.?arity // 2) < 2 {
             my $list = self.map(&by).eager;
-            self[(^pir::elements($list)).eager.sort(
+            self[$index_PARROT_RPA.sort(
                 -> $a, $b { $list[$a] cmp $list[$b] || $a <=> $b }
             )];
         }
         else {
             my $list = self.eager;
-            self[(^pir::elements($list)).eager.sort(
+            self[$index_PARROT_RPA.sort(
                 -> $a, $b { &by($list[$a],$list[$b]) || $a <=> $b }
             )];
         }
@@ -87,4 +90,7 @@ augment class Seq {
     }
 }
 
+multi sub sort (@x, :&by = &infix:<cmp>) { @x.sort(&by) }
+multi sub sort (&by, @x) { @x.sort(&by) }
+
 # vim: ft=perl6
-- 
1.6.6.1

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From @japhb

On Tue, 2010-04-13 at 05​:42 -0700, Ira Byerly wrote​:

Note that one test in t/spec/S32-list/sort.t fails...

not ok 4 \- array of mixed numbers including Inf/NaN
\#      got&#8203;: \[\-Inf, \-61/20, \-1e\-07, 1/10, 11/10, 2, 42, Inf, NaN\]
\# expected&#8203;: \[NaN, \-Inf, \-61/20, \-1e\-07, 1/10, 11/10, 2, 42, Inf\]

This is due to NaN sorting greater than Inf, rather than less than -Inf.
Since this is consistent with the Rakudo spaceship operator...

$ perl6 \-e 'say NaN \<=> Inf'
1

... I'm not sure that is appropriate to "fix" it in the code; it might be
better to change the test to match Rakudo's behavoir, or perhaps the test
should accept either >+Inf or <-Inf. The IEEE 754 standard just specifies
that than NaN should be "unordered".

In that case, the spec test should not care where the NaN sorts -- it
should be implementation-dependent (unless $Larry has declared
otherwise). However, that does not mean you should take NaN out of the
sort testing -- you still want to test that it can be "sorted" against
any other numeric value without invoking HCF (Halt and Catch Fire).

Probably the easiest solution is just to do the sort, check that it
contains a NaN somewhere, then grep the NaN out of the results and
compare the cleaned results against an expected list with no NaN in it.

-'f

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

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

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From @moritz

Thank you very much, I applied the patch after removing a trailing
whitespace and sanitizing the commit message a bit.

Cheers,
Moritz

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

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

@p6rt p6rt closed this as completed Apr 13, 2010
@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From @markjreed

Seems like both NaN <= $x and NaN >= $x should return false for any
$x, which makes me think NaN <=> $x should maybe return something
undefined?

On Tuesday, April 13, 2010, Geoffrey Broadwell <geoff@​broadwell.org> wrote​:

On Tue, 2010-04-13 at 05​:42 -0700, Ira Byerly wrote​:

Note that one test in t/spec/S32-list/sort.t fails...

    not ok 4 - array of mixed numbers including Inf/NaN
    #      got​: [-Inf, -61/20, -1e-07, 1/10, 11/10, 2, 42, Inf, NaN]
    # expected​: [NaN, -Inf, -61/20, -1e-07, 1/10, 11/10, 2, 42, Inf]

This is due to NaN sorting greater than Inf, rather than less than -Inf.
Since this is consistent with the Rakudo spaceship operator...

    $ perl6 -e 'say NaN <=> Inf'
    1

... I'm not sure that is appropriate to "fix" it in the code; it might be
better to change the test to match Rakudo's behavoir, or perhaps the test
should accept either >+Inf or <-Inf.  The IEEE 754 standard just specifies
that than NaN should be "unordered".

In that case, the spec test should not care where the NaN sorts -- it
should be implementation-dependent (unless $Larry has declared
otherwise).  However, that does not mean you should take NaN out of the
sort testing -- you still want to test that it can be "sorted" against
any other numeric value without invoking HCF (Halt and Catch Fire).

Probably the easiest solution is just to do the sort, check that it
contains a NaN somewhere, then grep the NaN out of the results and
compare the cleaned results against an expected list with no NaN in it.

-'f

--
Mark J. Reed <markjreed@​gmail.com>

@p6rt
Copy link
Author

p6rt commented Apr 13, 2010

From @kyleha

On Tue, Apr 13, 2010 at 4​:51 PM, Mark J. Reed <markjreed@​gmail.com> wrote​:

Seems like both NaN <= $x and NaN >= $x should return false for any
$x, which makes me think NaN <=> $x should maybe return something
undefined?

That reminds me of a bug I wrote in C++ one time. I had overloaded
comparisons in a way that didn't make any sense and caused the STL
QuickSort to run off the end of the array. Whether that's relevant to
Perl 6, I don't know. I'm just musing.

Kyle.

@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