Fix copy-pasta typo in regex utility for int parsing #363
Conversation
|
@swift-ci please test |
| @@ -58,7 +58,7 @@ extension IntegerParseStrategy: ParseStrategy { | |||
| if let value = parser.parseAsInt(substr, upperBound: &upperBound) { | |||
| let upperBoundInSubstr = String.Index(utf16Offset: upperBound, in: substr) | |||
| return (upperBoundInSubstr, Format.FormatInput(value)) | |||
| } else if let value = parser.parseAsInt(substr, upperBound: &upperBound) { | |||
| } else if let value = parser.parseAsDouble(substr, upperBound: &upperBound) { | |||
There was a problem hiding this comment.
Shouldn't this happen first? Otherwise, it will consume until . as an integer right?
There was a problem hiding this comment.
In the implementation at the above function (line 33) we also parse it as Int first, then try Double second. I believe the idea was that since it's a IntegerParseStrategy we'd try to parse it as Int if possible. From this perspective I think this fix is alright. It fixes the obvious copy pasta error, and the behavior is at least consistent with this other function.
That being said, in my playground it seems that both parseAsInt and parseAsDouble consumes the entire floating-point-like string. parseAsInt doesn't just stop at . either; rather, it just truncates the floating point string into an Int. That makes me wonder if that'd always succeed, and that this else if let value = parser.parseAsDouble clause actually never gets hit.
@gwynne did you see any test failures on your end that prompted this fix?
There was a problem hiding this comment.
Truthfully, I didn't even run the code in question; I was just looking at it in terms of the obvious copy-paste error 😅. I agree that it seems redundant to have both code paths in practice.
There was a problem hiding this comment.
I'm ok with merging it as is and revisit if we can remove this altogether separately.
There was a problem hiding this comment.
Looking again at this, I think we should just remove this code path. This has been in the wild since it was introduced and there hasn't been casualties (yet). We'll just come back to add back the logic to parse as Double if we ever come across a valid case.
There was a problem hiding this comment.
I concur, updating the PR 👍
| @@ -58,7 +58,7 @@ extension IntegerParseStrategy: ParseStrategy { | |||
| if let value = parser.parseAsInt(substr, upperBound: &upperBound) { | |||
| let upperBoundInSubstr = String.Index(utf16Offset: upperBound, in: substr) | |||
| return (upperBoundInSubstr, Format.FormatInput(value)) | |||
| } else if let value = parser.parseAsInt(substr, upperBound: &upperBound) { | |||
| } else if let value = parser.parseAsDouble(substr, upperBound: &upperBound) { | |||
There was a problem hiding this comment.
I'm ok with merging it as is and revisit if we can remove this altogether separately.
itingliu
dismissed
their stale review
Dismissing the review so @gwynne sees my latest comment
| @@ -32,8 +32,6 @@ extension IntegerParseStrategy: ParseStrategy { | |||
| let trimmedString = value._trimmingWhitespace() | |||
| if let v = parser.parseAsInt(trimmedString) { | |||
| return Format.FormatInput(v) | |||
| } else if let v = parser.parseAsDouble(trimmedString) { | |||
| return Format.FormatInput(clamping: Int64(v)) | |||
There was a problem hiding this comment.
This one I'm not so sure. We don't know if this code path is not currently being used even though I can't personally hit it. Should we just leave it as is for now?
|
@swift-ci please test |
Title kind've says it all 😅