-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow leading '+' in number strings #5589
Allow leading '+' in number strings #5589
Conversation
@@ -13,6 +13,8 @@ struct BigFloat < Float | |||
end | |||
|
|||
def initialize(str : String) | |||
# Strip leading '+' char to smooth out cases with strings like "+123" | |||
str = str.lchop('+') |
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.
It would be really simple to optimize this by simply adding 1 to the pointer if str.starts_with? '+'
. I think this optimization is so easy it might be a good idea to do it now to avoid the string copy and extra garbage.
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.
yeah, it's done in String#lchop
.
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.
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.
My bad indeed. str = str[1..-1] if str.starts_with?('+')
?
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.
Wouldn't be better to implement it directly in String#lchop
instead?
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.
It's not possible. All strings have 8 leading bytes: four bytes for the type_id, and four bytes for the string's size. So a substring must have this info too.
In any case, I consider this an optimization. We can merge this now and optimize it later.
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.
Yes, it's an optimization. However, we'll probably forget if this is merged as-is. This is a very simple optimization:
unsafe_str = str.to_unsafe
unsafe_str += 1 if str.starts_with?('+')
then use unsafe_str
. If you don't want to implement it, that's fine, it's just simple enough that now is as good a time as any to do this.
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.
I'd like to merge it as-is and leave this perf optimization to the most interested parties like yourself. Thank you.
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.
I general I hate it that we need such unsafe code. We should maybe work with slices in most cases unless we can prove a big performance loss.
This PR allows
+
leading character in numeric strings passed toBig*
class constructors, bringing coherence to the rest of theNumber
constructors and parity with Ruby:Crystal:
Ruby: