JRuby

Range#step - strange behavior with alpha range, numeric step

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: JRuby 1.0.2, JRuby 1.1b1
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    OS X, java 1.5, ruby 1.8.5 (2007-10-11 rev 3876) [i386-jruby1.1]
  • Number of attachments :
    3

Description

I'm not sure where's it's getting the "w" from. Should just be "a".

irb(main):001:0> r = Range.new('a', 'd')
=> "a".."d"
irb(main):002:0> r.step(100){ |e| p e }
"a"
"cw"
=> "a".."d"

Attachments

  1. File
    failing_test_for_1423.diff
    27/Oct/07 1:12 PM
    0.5 kB
    Mathias Biilmann Christensen
  2. File
    improved_patch_for_1423.diff
    28/Oct/07 8:04 AM
    2 kB
    Mathias Biilmann Christensen
  3. File
    patch_for_1423.diff
    27/Oct/07 1:13 PM
    1 kB
    Mathias Biilmann Christensen

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Damian Steer added a comment -

I know why: 'cw' is the 100th successor of 'a', it seems. Try:

x= 'a'; 100.times { x = x.succ; p x }

So it's printing the last element when (I assume from MRI) it shouldn't.

Show
Damian Steer added a comment - I know why: 'cw' is the 100th successor of 'a', it seems. Try: 
x= 'a'; 100.times { x = x.succ; p x }
So it's printing the last element when (I assume from MRI) it shouldn't.
Hide
Permalink
Daniel Berger added a comment -

Correct, it should not print any element beyond the first if the step size is longer than the range length. It seems to be getting confused if you provide a step size beyond the "natural" end of the datatype in question. For example:

irb(main):001:0> r = Range.new('a', 'd')
=> "a".."d"
irb(main):002:0> r.step(2){ |e| p e }
"a"
"c"
=> "a".."d"
irb(main):003:0> r.step(10){ |e| p e }
"a"
=> "a".."d"
irb(main):004:0> r.step(24){ |e| p e }
"a"
=> "a".."d"
irb(main):005:0> r.step(27){ |e| p e }
"a"
"ab"
"bc"
"cd"
=> "a".."d"

So, in this case, it goes wonky if you go off the edge of the number characters in English.

Dan

Show
Daniel Berger added a comment - Correct, it should not print any element beyond the first if the step size is longer than the range length. It seems to be getting confused if you provide a step size beyond the "natural" end of the datatype in question. For example: 
irb(main):001:0> r = Range.new('a', 'd')
=> "a".."d"
irb(main):002:0> r.step(2){ |e| p e }
"a"
"c"
=> "a".."d"
irb(main):003:0> r.step(10){ |e| p e }
"a"
=> "a".."d"
irb(main):004:0> r.step(24){ |e| p e }
"a"
=> "a".."d"
irb(main):005:0> r.step(27){ |e| p e }
"a"
"ab"
"bc"
"cd"
=> "a".."d"
So, in this case, it goes wonky if you go off the edge of the number characters in English. Dan
Hide
Permalink
Charles Oliver Nutter added a comment -

Ok, the problem here is the following algorithm used for iterating over non-numeric ranges:

while (currentObject.callMethod(context, compareMethod, (String)MethodIndex.NAMES.get(compareMethod), end).isTrue()) {
                block.yield(context, currentObject);
                
                for (int i = 0; i < stepSize; i++) {
                    currentObject = currentObject.callMethod(context, "succ");
                }
            }

The reason it goes wonky with step sizes greated than 26 is because 'aa' is < 'z' as far as string is concerned, so stepping continues. Obviously this is broken, but what's the correct semantics here? Using < or <= won't ever work for e.g. Strings, so that's obviously not the right test to use.

Show
Charles Oliver Nutter added a comment - Ok, the problem here is the following algorithm used for iterating over non-numeric ranges: 
while (currentObject.callMethod(context, compareMethod, (String)MethodIndex.NAMES.get(compareMethod), end).isTrue()) {
                block.yield(context, currentObject);
                
                for (int i = 0; i < stepSize; i++) {
                    currentObject = currentObject.callMethod(context, "succ");
                }
            }
The reason it goes wonky with step sizes greated than 26 is because 'aa' is < 'z' as far as string is concerned, so stepping continues. Obviously this is broken, but what's the correct semantics here? Using < or <= won't ever work for e.g. Strings, so that's obviously not the right test to use.
Hide
Permalink
Mathias Biilmann Christensen added a comment -

The "length" method suffers from the same problem, since it uses the same approach to figuring out when to stop calling succ:

irb(main):013:0> r = "a".."z"
=> "a".."z"
irb(main):014:0> r.length
=> 26
irb(main):015:0> r = "a".."aa"
=> "a".."aa"
irb(main):016:0> r.length
=> 1

In MRI Range has no length instance method, so I don't know if it's best to fix both step and length, or simply remove the buggy length method?

MRI takes a special approach for Strings in its step implementation.

I wrote some test cases for the step issue, and a patch that fixes the problem, based on the MRI approach of treating Strings as a separate case and I'll be uploading the diffs...

Show
Mathias Biilmann Christensen added a comment - The "length" method suffers from the same problem, since it uses the same approach to figuring out when to stop calling succ: irb(main):013:0> r = "a".."z" => "a".."z" irb(main):014:0> r.length => 26 irb(main):015:0> r = "a".."aa" => "a".."aa" irb(main):016:0> r.length => 1 In MRI Range has no length instance method, so I don't know if it's best to fix both step and length, or simply remove the buggy length method? MRI takes a special approach for Strings in its step implementation. I wrote some test cases for the step issue, and a patch that fixes the problem, based on the MRI approach of treating Strings as a separate case and I'll be uploading the diffs...
Hide
Permalink
Mathias Biilmann Christensen added a comment -

My first attempt at a patch for JRuby - Be gentle

Show
Mathias Biilmann Christensen added a comment - My first attempt at a patch for JRuby - Be gentle
Hide
Permalink
Charles Oliver Nutter added a comment -

Patch looks pretty clean to me, I'm testing it out now.

Show
Charles Oliver Nutter added a comment - Patch looks pretty clean to me, I'm testing it out now.
Hide
Permalink
Charles Oliver Nutter added a comment -

Thanks much for the patches, I've committed on trunk and 1.0 branch. Looking good!

Show
Charles Oliver Nutter added a comment - Thanks much for the patches, I've committed on trunk and 1.0 branch. Looking good!
Hide
Permalink
Mathias Biilmann Christensen added a comment -

As soon as I went to bed I actually came up with a slightly better way of doing this (I swear, my pillow has magic powers!!).

I've submitted a patch to the patch that will do the same job, but with one less call to == on each step, and slightly cleaner code.

Show
Mathias Biilmann Christensen added a comment - As soon as I went to bed I actually came up with a slightly better way of doing this (I swear, my pillow has magic powers!!). I've submitted a patch to the patch that will do the same job, but with one less call to == on each step, and slightly cleaner code.
Hide
Permalink
Charles Oliver Nutter added a comment -

Reopening so we don't forget to apply the "new and improved" patch.

Show
Charles Oliver Nutter added a comment - Reopening so we don't forget to apply the "new and improved" patch.
Hide
Permalink
Charles Oliver Nutter added a comment -

I applied the improved patch to trunk, but it failed to apply to 1.0 branch. Oh well...resolving.

Show
Charles Oliver Nutter added a comment - I applied the improved patch to trunk, but it failed to apply to 1.0 branch. Oh well...resolving.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.