-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix YamlDecoder and NaN tag #330
Conversation
Haha, my first draft for NaN/Inf parsing used regex too and after Tomek's nitpick I decided to simplify at the cost of precision with endsWith. It's also a bit faster than regex I think. Is this a problem you have encountered in a real scenario (the -XXXInf) or are you just fixing the possible edge case? I mean, I thought about it for a minute and decided it's rather improbable that user would expect the field to be a Float/Double and it would contain a string ending with Inf by chance. Proper perf-optimized impl would not use regex so it's a question whether we want to even take perf into consideration. |
If you attempt to parse a string "Reinf" as Float and succeed, then that's just a bug. There's no way around it. I don't mind if you optimize it further. I'm sure people, me included, would welcome that. (Generally speaking, optimizations should go hand in hand with benchmark, to show that they actually are optimizations.) But first it needs to be correct.
Fortunately I didn't stumble upon this bug in production. I was just reading the code and it figuratively jumped at me. |
@tgodzik we're back to regex matching, but this impl is cleaner than mine, I like it |
Btw this Any parsing is correct but very very slow. |
Do you have some numbers or any hint what actually is the bottleneck? Is it really the regex matching? That is something which won't be too difficult to fix. Maybe I could have a look at it myself if/when I have some time... |
I'd be happy to ditch the regexes and do "manual" comparisons, if it helps the PR to get merged. |
No need, it's fine, like I said, I went the same way on the original PR before review. Performance would have to be considered separately with a serious amount of pondering whether it's actually relevant in a yaml library. Does anyone actually parse or print yaml at scale? |
https://yaml.org/spec/1.2.2/#1032-tag-resolution
https://scastie.scala-lang.org/9DqmGdYWQwWA9OUmjqjbCg
Relates to
as[Any]
processing numbers with 32b precision only #314/cc @lbialy @OndrejSpanel