-
-
Notifications
You must be signed in to change notification settings - Fork 747
Optimized std.datetime.date.Date.fromISOString #5404
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
std/datetime/date.d
Outdated
| try | ||
| { | ||
| day = to!int(str[$ - 2 .. $]); | ||
| month = to!int(str[$ - 4 .. $ - 2]); |
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.
Relying on to!int throwing is too permissive. to!int accepts a sign, but it should be rejected in the day and month parts.
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.
Bah!
Back to the drawing board.
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.
Judging from the documentation, to!uint should work. Maybe add a test that checks against to!uint learning new tricks, like underscores.
|
@aG0aep6G Fixed the issue and managed to squeeze more performance out of it. Updated original comment |
| if (yearStr.length > 4) | ||
| { | ||
| enforce!DateTimeException(yearStr.startsWith('-', '+'), | ||
| text("Invalid ISO String: ", isoString)); |
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.
This can technically now throw a UTFException, because str is no longer guaranteed to be a dstring. So, you should either throw a representation call in before startsWith or just explicitly check the first element (since it's guaranteed to exist due to the previous checks for length).
| // using conversion to uint plus cast because it checks for +/- | ||
| // for us quickly while throwing ConvException | ||
| day = cast(int) to!uint(str[$ - 2 .. $]); | ||
| month = cast(int) to!uint(str[$ - 4 .. $ - 2]); |
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.
All of these to! calls (and the ones below) now autodecode, because str is no longer guaranteed to be a dstring, and so they risk throwing a UTFException. And I don't think that representation will work in this case. Try byCodeUnit, but I don't know if the bug preventing std.conv working with byCodeUnit was fixed yet. If not, given the clear speed-up, we can temporarily add a catch for UTFException so that we can rethrow a DateTimeException, but ultimately, none of this code should need to be doing anything with auto-decoding (at least outside of the error messages, which unfortunately do risk it with text, but I don't know what to do about that).
Yay! Thanks. |
|
Ping @jmdavis, since you approved the other PR can you take a look at this? |
Here's a summary of the changes
formatwithtextfor error messages, as format is slower due to string parsing when all you really need for this is concatenationdstringintrather thanubytes andshorts, as they use slower paths inside ofstd.conv.parsethanint.Here's the results
Result with second commit
Benchmark Code
Ping @jmdavis