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 String#upto #3701

Merged
merged 3 commits into from Jun 16, 2017

Conversation

Projects
None yet
2 participants
@ksss
Contributor

ksss commented Jun 14, 2017

The documented code didn't working as expected.

"9".upto("11").to_a   #=> []
"25".upto("5").to_a   #=> ["25"]
"07".upto("11").to_a  #=> ["07", "08", "09", "10", "11"]

I've fixed like this.

"9".upto("11").to_a   #=> ["9", "10", "11"]
"25".upto("5").to_a   #=> []
"07".upto("11").to_a  #=> ["07", "08", "09", "10", "11"]

The ruby/spec for String#upto passed all without Enumerator#size (using by mruby-spec)

https://github.com/ruby/spec/blob/97fe8c7cfd7a7e4833ae2edab4df21e3ea6bc093/core/string/upto_spec.rb

before

$ make TESTS=spec/core/string/upto
...
mruby 1.2.0 (2015-11-17)

1)
String#upto doesn't call block with self even if self is less than stop but stop length is less than self length FAILED
Expected ["25"] to equal []
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:20:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each

2)
String#upto tries to convert other to string using to_str ERROR
TypeError: non float value
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrbgems/mruby-string-ext/mrblib/string.rb:341:in upto
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:51:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each

3)
String#upto returns non-alphabetic characters in the ASCII range for single letters FAILED
Expected ["9"] to equal ["9", ":", ";", "<", "=", ">", "?", "@", "A"]
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:66:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each

4)
String#upto on sequence of numbers calls the block as Integer#upto FAILED
Expected [] to equal ["8", "9", "10", "11"]
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:79:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each

5)
String#upto when no block is given returned Enumerator size should return nil ERROR
NoMethodError: undefined method 'size' for #<Enumerator:0x7fa215023510>
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:93:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
[- | ==================100%================== | 00:00:00]      3     F      2     E

Finished in 0.012235 seconds

1 file, 15 examples, 16 expectations, 3 failures, 2 errors, 0 tagged
make: *** [run] Error 1

after

$ make TESTS=spec/core/string/upto
...
mruby 1.2.0 (2015-11-17)

1)
String#upto when no block is given returned Enumerator size should return nil ERROR
NoMethodError: undefined method 'size' for #<Enumerator:0x7ff048057300>
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:93:in protect
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/enum.rb:26:in all?
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/numeric.rb:77:in times
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
/Users/ksss/src/github.com/ksss/mruby-spec/spec/core/string/upto_spec.rb:98
/Users/ksss/src/github.com/ksss/mruby-spec/mruby/mrblib/array.rb:17:in each
[- | ==================100%================== | 00:00:00]      0     F      1     E

Finished in 0.007707 seconds

1 file, 15 examples, 19 expectations, 0 failures, 1 error, 0 tagged
make: *** [run] Error 1

ksss added some commits Jun 14, 2017

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Jun 14, 2017

Member

Thank you. But under the current implementation, we want to avoid using mrb_yield from C code. Using break within a block may cause unexpected result.

Member

matz commented Jun 14, 2017

Thank you. But under the current implementation, we want to avoid using mrb_yield from C code. Using break within a block may cause unexpected result.

@ksss

This comment has been minimized.

Show comment
Hide comment
@ksss

ksss Jun 14, 2017

Contributor

@matz Should I write in Ruby?
It's somewhat inefficient.

class String
  def upto(other_str, excl=false, &block)
    return to_enum :upto, other_str, excl unless block
    unless other_str = String.try_convert(other_str)
      raise TypeError, "no implicit conversion of #{other_str.class} into String"
    end

    # single character
    if bytesize == 1 && other_str.bytesize == 1
      c = bytes[0]
      e = other_str.bytes[0]
      return self if c > e || excl && c == e
      while 1
        block.call(c.chr)
        break if !excl && c == e
        c += 1
        break if excl && c == e
      end

      return self
    end

    # both edges are all digits
    if self != "0" && other_str != "0" && (b = self.to_i) > 0 && (e = other_str.to_i) > 0
      min_width = length
      max_width = other_str.length
      while b <= e
        break if excl && b == e
        bs = b.to_s
        bs = "#{"0" * (min_width - bs.length)}#{bs}" if min_width > bs.length
        block.call(bs)
        b += 1
      end

      return self
    end

    # normal case
    n = self <=> other_str
    return self if n > 0 || (excl && n == 0)
    after_end = other_str.succ
    current = dup
    while current != after_end
      nxt = nil
      nxt = current.succ if excl || current != other_str
      block.call(current)
      break if nxt.nil?
      current = nxt
      break if excl && current == other_str
      break if current.length > other_str.length || current.empty?
    end

    self
  end
end

t.rb

t = Time.now
"000001".upto("100000").each {}
p Time.now - t

t = Time.now
"aaa".upto("zzz").each {}
p Time.now - t

C implementation

$ mruby t.rb
0.112704
0.056333

Ruby implementation

$ mruby t.rb
0.416623
0.121773
Contributor

ksss commented Jun 14, 2017

@matz Should I write in Ruby?
It's somewhat inefficient.

class String
  def upto(other_str, excl=false, &block)
    return to_enum :upto, other_str, excl unless block
    unless other_str = String.try_convert(other_str)
      raise TypeError, "no implicit conversion of #{other_str.class} into String"
    end

    # single character
    if bytesize == 1 && other_str.bytesize == 1
      c = bytes[0]
      e = other_str.bytes[0]
      return self if c > e || excl && c == e
      while 1
        block.call(c.chr)
        break if !excl && c == e
        c += 1
        break if excl && c == e
      end

      return self
    end

    # both edges are all digits
    if self != "0" && other_str != "0" && (b = self.to_i) > 0 && (e = other_str.to_i) > 0
      min_width = length
      max_width = other_str.length
      while b <= e
        break if excl && b == e
        bs = b.to_s
        bs = "#{"0" * (min_width - bs.length)}#{bs}" if min_width > bs.length
        block.call(bs)
        b += 1
      end

      return self
    end

    # normal case
    n = self <=> other_str
    return self if n > 0 || (excl && n == 0)
    after_end = other_str.succ
    current = dup
    while current != after_end
      nxt = nil
      nxt = current.succ if excl || current != other_str
      block.call(current)
      break if nxt.nil?
      current = nxt
      break if excl && current == other_str
      break if current.length > other_str.length || current.empty?
    end

    self
  end
end

t.rb

t = Time.now
"000001".upto("100000").each {}
p Time.now - t

t = Time.now
"aaa".upto("zzz").each {}
p Time.now - t

C implementation

$ mruby t.rb
0.112704
0.056333

Ruby implementation

$ mruby t.rb
0.416623
0.121773
@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Jun 14, 2017

Member

I am trying to solve the issue, but at the moment, I don't recommend to use mrb_yield in your C code.

Member

matz commented Jun 14, 2017

I am trying to solve the issue, but at the moment, I don't recommend to use mrb_yield in your C code.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Jun 14, 2017

Member

I am talking about #3359

Member

matz commented Jun 14, 2017

I am talking about #3359

matz added a commit that referenced this pull request Jun 16, 2017

@matz matz merged commit 4d9ca27 into mruby:master Jun 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

matz added a commit that referenced this pull request Jun 16, 2017

Use `mrb_str_new()` instead of `malloc()`; ref #3701
Otherwise the function may terminate and cause memory leaks.

matz added a commit that referenced this pull request Jun 16, 2017

@ksss ksss deleted the ksss:string-upto branch Jun 16, 2017

@ksss

This comment has been minimized.

Show comment
Hide comment
@ksss

ksss Jun 16, 2017

Contributor

Thank you so much!

Contributor

ksss commented Jun 16, 2017

Thank you so much!

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