Skip to content

[stdlib] Fix Int(_:radix:) accepting unintentional cases (SR-187) #613

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.

Sign up for GitHub

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

Merged
gribozavr merged 6 commits into from
Dec 21, 2015
Merged

[stdlib] Fix Int(_:radix:) accepting unintentional cases (SR-187) #613

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0998e38
[stdlib] Fix internal doc comment
PatrickPijnappel Dec 16, 2015
8d1261e
[stdlib] Fix Int(_:radix:) accepting unintentional cases (SR-187)
PatrickPijnappel Dec 16, 2015
58bc25b
[stdlib] Add comment
PatrickPijnappel Dec 16, 2015
e6b08b9
Add tests for Int(_:radix) accepting unintentional cases (SR-187)
PatrickPijnappel Dec 17, 2015
4faa43a
[stdlib] Adjust formatting to fit 80 colums
PatrickPijnappel Dec 21, 2015
17a5845
[stdlib] Add terminating period to comments
PatrickPijnappel Dec 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 30 additions & 26 deletions stdlib/public/core/IntegerParsing.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,21 +67,20 @@ internal func _parseUnsignedAsciiAsUIntMax(
/// non-negative number <= `maximum`, return that number. Otherwise,
/// return `nil`.
///
/// - Note: If `text` begins with `"+"` or `"-"`, even if the rest of
/// the characters are `"0"`, the result is `nil`.
/// - Note: For text matching the regular expression "-0+", the result
/// is `0`, not `nil`.
internal func _parseAsciiAsUIntMax(
u16: String.UTF16View, _ radix: Int, _ maximum: UIntMax
utf16: String.UTF16View, _ radix: Int, _ maximum: UIntMax
) -> UIntMax? {
if u16.isEmpty { return nil }
let c = u16.first
if _fastPath(c != _ascii16("-")) {
let unsignedText
= c == _ascii16("+") ? u16.dropFirst() : u16
return _parseUnsignedAsciiAsUIntMax(unsignedText, radix, maximum)
}
else {
return _parseAsciiAsIntMax(u16, radix, 0) == 0 ? 0 : nil
}
if utf16.isEmpty { return nil }
// Parse (optional) sign
Copy link
Contributor

@gribozavr gribozavr Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to end inline comments with a period to remove the possible ambiguity between a complete comment and a comment that was left unfinished (for example, because the engineer was distracted).

let (digitsUTF16, hasMinus) = _parseOptionalAsciiSign(utf16)
// Parse digits
guard let result = _parseUnsignedAsciiAsUIntMax(digitsUTF16, radix, maximum) else { return nil }
// Disallow < 0
if hasMinus && result != 0 { return nil }
// Return
Copy link
Contributor

@gribozavr gribozavr Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think // Return is a useful comment :)

Copy link
Contributor Author

@PatrickPijnappel PatrickPijnappel Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments are intended as headings for code grouping (instead of newlines) so you can quickly skim the algorithm and it's structure/flow just from those headings. But I agree // Return in itself it not a very useful comment :). I'll can replace it by a slightly more useful heading, e.g. // Valid result. (to indicate the algorithm early-returned all invalids), or remove it (/replace it by a newline) if you prefer ;).

return result
}

/// If text is an ASCII representation in the given `radix` of a
Expand All @@ -91,23 +90,28 @@ internal func _parseAsciiAsUIntMax(
/// - Note: For text matching the regular expression "-0+", the result
/// is `0`, not `nil`.
internal func _parseAsciiAsIntMax(
u16: String.UTF16View, _ radix: Int, _ maximum: IntMax
utf16: String.UTF16View, _ radix: Int, _ maximum: IntMax
) -> IntMax? {
_sanityCheck(maximum >= 0, "maximum should be non-negative")
if utf16.isEmpty { return nil }
// Parse (optional) sign
let (digitsUTF16, hasMinus) = _parseOptionalAsciiSign(utf16)
// Parse digits
let absValueMax = UIntMax(bitPattern: maximum) + (hasMinus ? 1 : 0) // E.g. Int8's range is -128...127
guard let absValue = _parseUnsignedAsciiAsUIntMax(digitsUTF16, radix, absValueMax) else { return nil }
// Return signed result
return IntMax(bitPattern: hasMinus ? 0 &- absValue : absValue)
}

if u16.isEmpty { return nil }

// Drop any leading "-"
let negative = u16.first == _ascii16("-")
let absResultText = negative ? u16.dropFirst() : u16

let absResultMax = UIntMax(bitPattern: maximum) + (negative ? 1 : 0)

// Parse the result as unsigned
if let absResult = _parseAsciiAsUIntMax(absResultText, radix, absResultMax) {
return IntMax(bitPattern: negative ? 0 &- absResult : absResult)
/// Strip an optional single leading ASCII plus/minus sign from `utf16`.
private func _parseOptionalAsciiSign(
utf16: String.UTF16View
) -> (digitsUTF16: String.UTF16View, isMinus: Bool) {
switch utf16.first {
case _ascii16("-")?: return (utf16.dropFirst(), true)
case _ascii16("+")?: return (utf16.dropFirst(), false)
default: return (utf16, false)
}
return nil
}

//===--- Loop over all integer types --------------------------------------===//
Expand Down
4 changes: 4 additions & 0 deletions test/1_stdlib/NumericParsing.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ tests.test("${Self}/success") {
expectEqual(nil, ${Self}("${minValue - 1}"))
expectEqual(nil, ${Self}("\u{1D7FF}")) // MATHEMATICAL MONOSPACE DIGIT NINE

// Cases that should fail to parse
expectEqual(nil, ${Self}("--0")) // Zero w/ repeated plus
expectEqual(nil, ${Self}("-+5")) // Non-zero with -+

// Do more exhaustive testing
% for radix in radices_to_test:
% for n in required_values + range(
Expand Down