-
Notifications
You must be signed in to change notification settings - Fork 44
[Feature #21943] Add StringScanner#integer_at
#193
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
base: master
Are you sure you want to change the base?
Changes from all commits
c20f4a7
efe8d0f
d74afd1
485f27c
ef6281d
6052143
f64fdd8
6a55781
67a10ae
9ab1172
960130b
0065ecf
d89e54b
4d3583f
e686711
0f2ad2a
433fd87
72c0426
0c5c88e
674e32c
6b44dbd
b6fe693
a078c0d
b1b135a
db383e9
a421b12
a81aa3e
d07b086
1ae5772
04a7f84
21c6be8
e6e5c27
d5b3651
ba15508
9efb4d2
f51e1cf
764a4a1
0f67657
7f19293
e4c6a1e
9285091
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1621,6 +1621,37 @@ name_to_backref_number(struct re_registers *regs, VALUE regexp, const char* name | |||||||||||||||||||||||||||||||||||||||||||
| rb_long2int(name_end - name), name); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* Resolve capture group index from Integer, Symbol, or String. | ||||||||||||||||||||||||||||||||||||||||||||
| * Returns the resolved register index, or -1 if unmatched/out of range. */ | ||||||||||||||||||||||||||||||||||||||||||||
| static long | ||||||||||||||||||||||||||||||||||||||||||||
| resolve_capture_index(struct strscanner *p, VALUE specifier) | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| const char *name; | ||||||||||||||||||||||||||||||||||||||||||||
| long i; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (! MATCHED_P(p)) return -1; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| switch (TYPE(specifier)) { | ||||||||||||||||||||||||||||||||||||||||||||
| case T_SYMBOL: | ||||||||||||||||||||||||||||||||||||||||||||
| specifier = rb_sym2str(specifier); | ||||||||||||||||||||||||||||||||||||||||||||
| /* fall through */ | ||||||||||||||||||||||||||||||||||||||||||||
| case T_STRING: | ||||||||||||||||||||||||||||||||||||||||||||
| RSTRING_GETMEM(specifier, name, i); | ||||||||||||||||||||||||||||||||||||||||||||
| i = name_to_backref_number(&(p->regs), p->regex, name, name + i, rb_enc_get(specifier)); | ||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||
| i = NUM2LONG(specifier); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (i < 0) | ||||||||||||||||||||||||||||||||||||||||||||
| i += p->regs.num_regs; | ||||||||||||||||||||||||||||||||||||||||||||
| if (i < 0) return -1; | ||||||||||||||||||||||||||||||||||||||||||||
| if (i >= p->regs.num_regs) return -1; | ||||||||||||||||||||||||||||||||||||||||||||
| if (p->regs.beg[i] == -1) return -1; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return i; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||
| * :markup: markdown | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1695,30 +1726,12 @@ name_to_backref_number(struct re_registers *regs, VALUE regexp, const char* name | |||||||||||||||||||||||||||||||||||||||||||
| static VALUE | ||||||||||||||||||||||||||||||||||||||||||||
| strscan_aref(VALUE self, VALUE idx) | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| const char *name; | ||||||||||||||||||||||||||||||||||||||||||||
| struct strscanner *p; | ||||||||||||||||||||||||||||||||||||||||||||
| long i; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| GET_SCANNER(self, p); | ||||||||||||||||||||||||||||||||||||||||||||
| if (! MATCHED_P(p)) return Qnil; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| switch (TYPE(idx)) { | ||||||||||||||||||||||||||||||||||||||||||||
| case T_SYMBOL: | ||||||||||||||||||||||||||||||||||||||||||||
| idx = rb_sym2str(idx); | ||||||||||||||||||||||||||||||||||||||||||||
| /* fall through */ | ||||||||||||||||||||||||||||||||||||||||||||
| case T_STRING: | ||||||||||||||||||||||||||||||||||||||||||||
| RSTRING_GETMEM(idx, name, i); | ||||||||||||||||||||||||||||||||||||||||||||
| i = name_to_backref_number(&(p->regs), p->regex, name, name + i, rb_enc_get(idx)); | ||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||
| i = NUM2LONG(idx); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (i < 0) | ||||||||||||||||||||||||||||||||||||||||||||
| i += p->regs.num_regs; | ||||||||||||||||||||||||||||||||||||||||||||
| if (i < 0) return Qnil; | ||||||||||||||||||||||||||||||||||||||||||||
| if (i >= p->regs.num_regs) return Qnil; | ||||||||||||||||||||||||||||||||||||||||||||
| if (p->regs.beg[i] == -1) return Qnil; | ||||||||||||||||||||||||||||||||||||||||||||
| i = resolve_capture_index(p, idx); | ||||||||||||||||||||||||||||||||||||||||||||
| if (i < 0) return Qnil; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return extract_range(p, | ||||||||||||||||||||||||||||||||||||||||||||
| adjust_register_position(p, p->regs.beg[i]), | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1852,6 +1865,171 @@ strscan_values_at(int argc, VALUE *argv, VALUE self) | |||||||||||||||||||||||||||||||||||||||||||
| return new_ary; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* Max decimal digits guaranteed to fit in long without overflow check. | ||||||||||||||||||||||||||||||||||||||||||||
| * floor(log10(INT64_MAX)) = 18, floor(log10(INT32_MAX)) = 9 */ | ||||||||||||||||||||||||||||||||||||||||||||
| #define INT64_DECIMAL_SAFE_DIGITS 18 | ||||||||||||||||||||||||||||||||||||||||||||
| #define INT32_DECIMAL_SAFE_DIGITS 9 | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* Fast path for base-10 integer parsing without temporary String allocation. | ||||||||||||||||||||||||||||||||||||||||||||
| * Accepts digits and optional underscores (Ruby String#to_i semantics). | ||||||||||||||||||||||||||||||||||||||||||||
| * Returns a Fixnum/Integer VALUE on success, or Qundef to signal fall-through | ||||||||||||||||||||||||||||||||||||||||||||
| * to the general path (non-decimal, bignum, or non-numeric input). */ | ||||||||||||||||||||||||||||||||||||||||||||
| static inline VALUE | ||||||||||||||||||||||||||||||||||||||||||||
| parse_decimal_fast(const char *ptr, long len) | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| long digits_start = 0; | ||||||||||||||||||||||||||||||||||||||||||||
| bool negative = false; | ||||||||||||||||||||||||||||||||||||||||||||
| long digit_count = 0; | ||||||||||||||||||||||||||||||||||||||||||||
| bool valid = true; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (ptr[0] == '-') { negative = true; digits_start = 1; } | ||||||||||||||||||||||||||||||||||||||||||||
| else if (ptr[0] == '+') { digits_start = 1; } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* Validate: only digits and underscores (not leading/trailing/consecutive) */ | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| long i; | ||||||||||||||||||||||||||||||||||||||||||||
| bool prev_underscore = true; /* treat start as underscore to reject leading _ */ | ||||||||||||||||||||||||||||||||||||||||||||
| for (i = digits_start; i < len; i++) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (ptr[i] >= '0' && ptr[i] <= '9') { | ||||||||||||||||||||||||||||||||||||||||||||
| digit_count++; | ||||||||||||||||||||||||||||||||||||||||||||
| prev_underscore = false; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| else if (ptr[i] == '_' && !prev_underscore) { | ||||||||||||||||||||||||||||||||||||||||||||
| prev_underscore = true; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||||||
| valid = false; | ||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if (prev_underscore && digit_count > 0) valid = false; /* trailing _ */ | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (!valid || digit_count == 0) return Qundef; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| /* Skip leading zeros to get effective digit count */ | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| long first_nonzero = digits_start; | ||||||||||||||||||||||||||||||||||||||||||||
| long effective_digits; | ||||||||||||||||||||||||||||||||||||||||||||
| long i; | ||||||||||||||||||||||||||||||||||||||||||||
| while (first_nonzero < len && (ptr[first_nonzero] == '0' || ptr[first_nonzero] == '_')) | ||||||||||||||||||||||||||||||||||||||||||||
| first_nonzero++; | ||||||||||||||||||||||||||||||||||||||||||||
| effective_digits = 0; | ||||||||||||||||||||||||||||||||||||||||||||
| for (i = first_nonzero; i < len; i++) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (ptr[i] != '_') effective_digits++; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (effective_digits <= (sizeof(long) >= 8 ? INT64_DECIMAL_SAFE_DIGITS : INT32_DECIMAL_SAFE_DIGITS)) { | ||||||||||||||||||||||||||||||||||||||||||||
| long result = 0; | ||||||||||||||||||||||||||||||||||||||||||||
| for (i = first_nonzero; i < len; i++) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (ptr[i] != '_') | ||||||||||||||||||||||||||||||||||||||||||||
| result = result * 10 + (ptr[i] - '0'); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| if (negative) result = -result; | ||||||||||||||||||||||||||||||||||||||||||||
| return LONG2NUM(result); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| /* One more digit than safe: may still fit in long with overflow check */ | ||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of combining validation/counting/safe_digits_branch/one_more_digit_branch to a single for-loop like this? for (k = j; k < len; k++) { //single moderate-fast path
if (invalid) stop_parsing_or_fallback_to_slow_path;
if (result > (limit - d) / 10) fallback_to_slow_path; // always check overflow
result = result * 10 + d;
}It always checks overflow, which is an overhead. It always performs multiplication even for Compared to this, the current code is really optimized to remove all the overhead that the simple for-loop has.
I just want to know if the performance improvement of these optimizations are really worth adding complexity to the code. If the overflow check overhead needs to be reduced, something like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’d like to hear your thoughts on this comment. Regarding However,
I don’t think I need to respond.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's consider with numbers. BTW, why did you close ruby/ruby#16322 ? If |
||||||||||||||||||||||||||||||||||||||||||||
| else if (effective_digits == (sizeof(long) >= 8 ? INT64_DECIMAL_SAFE_DIGITS + 1 : INT32_DECIMAL_SAFE_DIGITS + 1)) { | ||||||||||||||||||||||||||||||||||||||||||||
| unsigned long result = 0; | ||||||||||||||||||||||||||||||||||||||||||||
| unsigned long limit = negative | ||||||||||||||||||||||||||||||||||||||||||||
| ? (unsigned long)LONG_MAX + 1 | ||||||||||||||||||||||||||||||||||||||||||||
| : (unsigned long)LONG_MAX; | ||||||||||||||||||||||||||||||||||||||||||||
| bool overflow = false; | ||||||||||||||||||||||||||||||||||||||||||||
| for (i = first_nonzero; i < len; i++) { | ||||||||||||||||||||||||||||||||||||||||||||
| if (ptr[i] != '_') { | ||||||||||||||||||||||||||||||||||||||||||||
| unsigned long d = ptr[i] - '0'; | ||||||||||||||||||||||||||||||||||||||||||||
| /* Pre-check before multiply to avoid unsigned long wraparound on | ||||||||||||||||||||||||||||||||||||||||||||
| * 32-bit platforms, where 10-digit values can exceed ULONG_MAX. */ | ||||||||||||||||||||||||||||||||||||||||||||
| if (result > (limit - d) / 10) { | ||||||||||||||||||||||||||||||||||||||||||||
| overflow = true; | ||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1937
to
+1946
|
||||||||||||||||||||||||||||||||||||||||||||
| bool overflow = false; | |
| for (; j < len; j++) { | |
| if (ptr[j] != '_') { | |
| unsigned long d = ptr[j] - '0'; | |
| result = result * 10 + d; | |
| if (result > limit) { | |
| overflow = true; | |
| break; | |
| } | |
| unsigned long limit_div10 = limit / 10; | |
| unsigned long limit_mod10 = limit % 10; | |
| bool overflow = false; | |
| for (; j < len; j++) { | |
| if (ptr[j] != '_') { | |
| unsigned long d = (unsigned long)(ptr[j] - '0'); | |
| if (result > limit_div10 || | |
| (result == limit_div10 && d > limit_mod10)) { | |
| overflow = true; | |
| break; | |
| } | |
| result = result * 10 + d; |
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.
e6e5c27 fixed it
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.
The comment for
resolve_capture_indexsays it returns -1 for unmatched/out-of-range cases, butname_to_backref_numberraisesIndexErrorfor unknown named captures. Please update the comment to reflect that the function may raise for an undefined group name.