-
Notifications
You must be signed in to change notification settings - Fork 43
StringScanner#scan_integer support base 16 integers #116
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
Conversation
958d211 to
e68e38f
Compare
ff7f16a to
89292ec
Compare
|
Hum, Truffle CI is broken, because now @eregon any idea how to fix it? Is there even value in testing TruffleRuby in this repo if the implementation is in Truffle itself? |
|
I pushed caaaf94 to fix TruffleRuby CI, but it's quite ugly. |
|
I want to remove TruffleRuby related codes from this repository but how about moving |
If that's what you prefer, it's possible yes. |
Followup: ruby#115 `scan_integer` is now implemented in Ruby as to efficiently handle keyword arguments without allocating a Hash. Given the goal of `scan_integer` is to more effciently parse integers without having to allocate an intermediary object, using `rb_scan_args` would defeat the purpose. Additionally, the C implementation now uses `rb_isdigit` and `rb_isxdigit`, because on Windows `isdigit` is locale dependent.
caaaf94 to
5b950ca
Compare
|
|
||
| rb_define_method(StringScanner, "named_captures", strscan_named_captures, 0); | ||
|
|
||
| rb_require("strscan/strscan"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to use rb_funcall(rb_mKernel, rb_intern("require"), 1, rb_str_new_cstr("strscan/strscan")) or something because RubyGems may replace rb_require().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Other gems like psych, json etc use rb_require directly without known issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't know it.
OK. Let's use this. I hope that this is a temporary workaround...
|
Thanks. |
|
Thank you. Let me know if there any more features you'd like for the initial version, and if some use cases come up in the future I'm happy to help implement them. |
|
I don't have any more requests for the initial version. I'll release a new version in a few weeks. |
|
For the record: # frozen_string_literal: true
require 'strscan'
require 'benchmark/ips'
source = 10_000.times.map { rand(9999999).to_s }.join(",").freeze
def scan_to_i(source)
scanner = StringScanner.new(source)
while number = scanner.scan(/\d+/)
number.to_i
scanner.skip(",")
end
end
def scan_integer(source)
scanner = StringScanner.new(source)
while scanner.scan_integer
scanner.skip(",")
end
end
Benchmark.ips do |x|
x.report("scan.to_i") { scan_to_i(source) }
x.report("scan_integer") { scan_integer(source) }
x.compare!
endI was hopping for more, but 2.3x is already a nice gain, and profile shows there is some optimizations we could do in the Ruby side: https://share.firefox.dev/49bOcTn |
Followup: #115
scan_integeris now implemented in Ruby as to efficiently handle keyword arguments without allocating a Hash. Given the goal ofscan_integeris to more effciently parse integers without having to allocate an intermediary object, usingrb_scan_argswould defeat the purpose.Additionally, the C implementation now uses
rb_isdigitandrb_isxdigit, because on Windowsisdigitis locale dependent.cc @kou