Fix Range#sum optimized version #6487

Merged
merged 1 commit into from May 25, 2012

2 participants

@avakhov

At 1bd4d1c was added Range#sum
optimized version for arithmetic progressions. This improvment injected
a defect with not integer range boundaries. The defect was fixed by
e0adfa8. The second commit really
disabled optimization at all because in Ruby integer-valued numbers are
instances of Fixnum and Bignum classes. We should #use is_a?
(#kind_of?) method instead #instance_of? to check if value is numerical:

1.class                 # => Fixnum
1.instance_of?(Integer) # => false
1.is_a?(Integer)        # => true

-100_000_000_000.class                 # => Bignum
-100_000_000_000.instance_of?(Integer) # => false
-100_000_000_000.is_a?(Integer)        # => true

Moreover original implementation of Range#sum has a defect with reverse
range boundaries. If the first boundary is less than the second range is
empty. Current commit fixes and tests this case too.

@avakhov avakhov Fix Range#sum optimized version
At 1bd4d1c67459a91415ee73a8f55d2309c0d62a87 was added Range#sum
optimized version for arithmetic progressions. This improvment injected
a defect with not integer range boundaries. The defect was fixed by
e0adfa82c05f9c975005f102b4bcaebfcd17d241. The second commit really
disabled optimization at all because in Ruby integer-valued numbers are
instances of Fixnum and Bignum classes. We should #use is_a?
(#kind_of?) method instead #instance_of? to check if value is numerical:

    1.class                 # => Fixnum
    1.instance_of?(Integer) # => false
    1.is_a?(Integer)        # => true

    -100_000_000_000.class                 # => Bignum
    -100_000_000_000.instance_of?(Integer) # => false
    -100_000_000_000.is_a?(Integer)        # => true

Moreover original implementation of Range#sum has a defect with reverse
range boundaries. If the first boundary is less than the second range is
empty. Current commit fixes and tests this case too.
b4167d3
@rafaelfranca rafaelfranca merged commit 8e1d46c into rails:master May 25, 2012
@avakhov

Thanks! It was so fast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment