Adding TryParse pattern. - #76
Conversation
lemire
commented
Mar 11, 2021
@CarlVerret Might be nice to make sure that all public methods are documented. See https://stackoverflow.com/questions/17110006/how-to-find-uncommented-public-methods-using-resharper |
CarlVerret
commented
Mar 11, 2021
nice trick! |
lemire
commented
Mar 11, 2021
@CarlVerret So when can these functions throw?
Otherwise, we always succeed in the sense that we do not throw. Is this your understanding? |
CarlVerret
commented
Mar 12, 2021
If I am not mistaken, TryParse funcs should never throw as Parse func will always do. I've just pushed some minor adjustments. |
lemire
commented
Mar 12, 2021
@CarlVerret Ok. Yes, so I poorly worded my question. I meant the Parse functions, they will only throw on the cases that I have identified, correct? |
Uh oh!
There was an error while loading. Please reload this page.
| /// <returns>parsed double value </returns> | ||
| public static unsafe double ParseDouble(char* first, char* last, out int characters_consumed, NumberStyles styles = NumberStyles.Float, char decimal_separator = '.') | ||
| { | ||
| if (!TryParseDouble(first, last, out characters_consumed, out double result, styles, decimal_separator)) |
There was a problem hiding this comment.
Maybe swap the condition? The branch predictor (when there's no history) will take the fall-through path. So
if(TryParse(...)){returnresult;}ThrowArgumentException();thrownull;Note: the final throw null is just there to make the compiler happy, as otherwise there wouldn't be a return value from this code path.
Alternatively instead of throw null you can use return default. At codegen this will be dead code eliminated anyway.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,524 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
I consider this file as build-artifact, so it doesn't need to be checked in.
Not the scope for this PR, but as idea:
With https://dotnet.github.io/docfx you can create a static documentation site, which could be hosted here in the GH-pages to this project.
Additional markdown files, etc. can also be incorporated into that doc-build.
It's quite easy to automate and push the changes to GH-pages by CI.
There was a problem hiding this comment.
i'll make that an Issue, really intersting idea thanks for sharing!
Co-authored-by: Günther Foidl <gue@korporal.at>
Yes, this is how I've figured it. There's three cases who result throwing : null, empty and invalid input. |
| public static unsafe bool TryParseDouble(string s, out int characters_consumed, out double result, NumberStyles styles = NumberStyles.Float, char decimal_separator = '.') | ||
| { | ||
| if (string.IsNullOrEmpty(s)) | ||
| if (s == null) |
There was a problem hiding this comment.
This change is OK, but either flip the condition to if (s != null) (for the branch preditor) or (just came into my mind) take a sligth different approach:
returnTryParseDouble(s.AsSpan(),outcharacters_consumed,outresult, ...);so to forward to the span-based version.
In the case of null this results in a empty span, so that length will be 0 and algorithm returns (without throwing an null-ref exception).
There was a problem hiding this comment.
If followed your second idea, fowarding to span-based. I haven't noticed before that the asSpan function was an extension on "string?" ! This is really interesting thanks.
| private void Issue_74() | ||
| { | ||
| // Consumed=0 vs raising Exceptions | ||
| // Try parse should retrun false with consummed and result =0 |
lemire
left a comment
There was a problem hiding this comment.
Please correct the typo before merging.
Would fix#74 by adding a whole new set of TryParse functions.