Skip to content

Reimplement String#upto#3701

Merged
matz merged 3 commits into
mruby:masterfrom
ksss:string-upto
Jun 16, 2017
Merged

Reimplement String#upto#3701
matz merged 3 commits into
mruby:masterfrom
ksss:string-upto

Conversation

@ksss
Copy link
Copy Markdown
Contributor

@ksss 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

@matz
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
matz added a commit that referenced this pull request Jun 16, 2017
matz added a commit that referenced this pull request Jun 16, 2017
Otherwise the function may terminate and cause memory leaks.
@ksss ksss deleted the string-upto branch June 16, 2017 03:17
@ksss
Copy link
Copy Markdown
Contributor Author

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants