SR-1378: Rewrite _countFormatSpecifiers(_:) to handle positional specifiers #3555
Conversation
| // characters (percent, dollar, zero, and nine), we don't need to decode UTF-16. | ||
|
|
||
| enum ScanState { | ||
| case NoPercent |
There was a problem hiding this comment.
Not really a big deal here, as it's internal, but enum cases should be camelCase.
There was a problem hiding this comment.
I was still in Swift 2.2 mode, mentally at least. :-)
|
Overall LGTM. Couple minor comments. |
|
Thanks @rsfinn, and sorry for a much delayed review. |
| state = .noPercent | ||
| } | ||
| else if c >= zeroUTF16 && c <= nineUTF16 { | ||
| posValue = posValue * 10 + (Int(c) - Int(zeroUTF16)) |
There was a problem hiding this comment.
Just a though...
Since enums in Swift are more powerful than in C, it is possible to actually attach a posValue variable to the .scanningDigits case. So that it would look like case scanningDigits(current: UInt16). Which means that in the switch you would be able to get the posValue value right from the state itself like this case .scanningDigits(let current). This would eliminate one extra 'global' variable, and move it into state.
This approach can be take even further by representing all the now-global state inside the state, but that is probably unreasonable, because that would require multiple states to hold the values these states don't use, but merely pass around.
Integrating posValue into the .scanningDigits state, on the other hand, is quite natural, as the posValue is only used inside the code that handles this particular state.
What do you think?
There was a problem hiding this comment.
That sounds like a good idea. I'm still accustoming myself to Swift's idioms and that approach had not occurred to me.
I thought I had pushed my previous changes earlier, and this pull request page seems to reflect that; but I had a local file system issue after pushing my initial pull request and had to reconstruct my local repository, so maybe there's a problem with my configuration. I'll make this most recent change at my end and update this page when I've done so; if the update doesn't take here, feel free to go ahead and make the update at your end.
I'll be pleased to see this fix get into 3.0; thanks again for your feedback.
|
@rsfinn, thank you so much for your efforts! However, we can't repair these checks in Swift 3; rationale is in #3917. As it says there, doing this right introduces even more complexity than your patch (which wouldn't handle |
[pull] swiftwasm from main
What's in this pull request?
Rewrite
_countFormatSpecifiers(_:)to handle positional specifiers correctly.Resolved bug number: (SR-1378)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.