Complete implementation of NSScanner #2582

ms-jihua · 2017-04-20T00:50:03Z

Implemented the following functions/properties:
- locale
- copyWithZone:
- localizedScannerWithString:
- scanDecimal:
- scanHexDouble:
- scanHexFloat:
Added unit tests for NSScanner (there were none, including for previously implemented features)
Fixed a number of edge cases that were incorrect

Fixes #2083

ms-jihua · 2017-04-20T00:50:25Z

Frameworks/Foundation/NSScanner.mm

+                    if (!scannedPeriod) {
+                        scannedPeriod = true;
+                    } else {
+                        goto done_scanning_hex_double; // Stop scanning once a second period has been scanned


Since break inside a switch in a for loop breaks the switch and not the outer loop, this unfortunately seemed to be the best out of a group of bad options:

Add a done bool that is checked on every iteration: this causes ++i to occur once more than necessary unless it's the end of the string, making the set of _scanLocation = 1 at the end more complicated than necessary

Move the switch to a helper function: would need to manipulate both mantissa and exponent through out-pointers, seems unnecessarily messy

Add a continue to every 'good' case, then add a break outside the switch that the 'bad' cases can fall too: wwwwaayyy too messy

nit: I'm not sure if our coding standards handle gotos, but I think if this is the best option then we should make it follow the macro-style naming convention of ALL_CAPS so it really stands out.

DHowett-MSFT · 2017-04-20T00:53:38Z

Frameworks/Foundation/NSScanner.mm

        } else if (!hasSign && unicode == '+') {
            sign = 1;
-            hasSign = TRUE;
+            hasSign = YES;
        } else if (unicode >= '0' && unicode <= '9') {


iswdigit?

DHowett-MSFT · 2017-04-20T00:54:06Z

Frameworks/Foundation/NSScanner.mm

        } else if (unicode >= '0' && unicode <= '9') {
            if (!hasOverflow) {
                int c = unicode - '0';

                // Inspired by http://www.math.utoledo.edu/~dbastos/overflow.html
-                if ((long_long_MAX - c) / 10 < value)
-                    hasOverflow = TRUE;
+                if ((longLongMax - c) / 10 < value)


aaaa my eyes they are burning

nit: please fix these weirdos 😄

DHowett-MSFT · 2017-04-20T00:56:59Z

Frameworks/Foundation/NSScanner.mm

-                if ([_skipSet characterIsMember:unicode])
-                    state = STATE_SPACE;
-                else if (unicode == '0') {
+            case STATE_START:


can these states be unified?

(the first two)

DHowett-MSFT · 2017-04-20T00:57:57Z

Frameworks/Foundation/NSScanner.mm

    }

-    return TRUE;
+    return YES;
 }

 /**
 @Status Interoperable
 */
 - (BOOL)scanHexInt:(unsigned*)valuep {


can we implement scanHexInt in terms of scanHexLongLong? we'd have to accoutn for overflow behaviour, but it would kill half our hex reading code

DHowett-MSFT · 2017-04-20T00:58:38Z

Frameworks/Foundation/NSScanner.mm

-                            value = check;
-                        else {
-                            value = -1;
+                        if (0xF000000000000000 & value) {


doesn't this need ULL at the end? hmm

DHowett-MSFT · 2017-04-20T01:01:02Z

Frameworks/Foundation/NSScanner.mm

+    bool mantissaIsNegative = false;
+    bool exponentIsNegative = false;
+    bool scannedPeriod = false;
+    bool hasValue = false;


nit: your new code has hasValue as a bool but the old code has it as BOOL; standardize?

DHowett-MSFT · 2017-04-20T01:01:27Z

Frameworks/Foundation/NSScanner.mm

+
+done_scanning_hex_double:; // Allows breaking out of the loop from inside the switch
+    if (hasValue) {
+        if (result) {


is there an overflow case to be had for decimals?

you mean besides the exponent getting too large and causing ldexp to return HUGE_VAL or 0?

if the mantissa becomes too precise to represent correctly, the latter (least significant) bits get chopped off during the double operations in STATE_MANTISSA

DHowett-MSFT · 2017-04-20T01:02:02Z

Frameworks/Foundation/NSScanner.mm

-    return StubReturn();
-}
+- (BOOL)isAtEnd {
+    @synchronized(self) {


none of the other APIs are @synchronized... should they be?

NSScanner is in the list of thread-unsafe Foundation classes.

DHowett-MSFT · 2017-04-20T01:02:46Z

tests/unittests/Foundation/NSScannerTests.mm

+
+static const double c_floatingPtTolerance = 0.00001;
+
+// Helper template function that calls a scan_______ function specified by the selector and does the nececssary casts


nit: nececssary

DHowett-MSFT · 2017-04-20T01:04:43Z

tests/unittests/Foundation/NSScannerTests.mm

+}
+
+TEST(NSScanner, ScanHexLongLong) {
+    testScanHexIntegral<unsigned long long>(@selector(scanHexLongLong:));


I'd like to see an additional test for something that fits in 64 but not 32; this doesn't have that

msft-Jeyaram · 2017-04-20T01:22:52Z

Frameworks/Foundation/NSScanner.mm

+
+                } else if (unicode >= 'a' && unicode <= 'f') {
+                    if (scannedPeriod) {
+                        mantissa += (mantissaDigitPlace /= 16) * (unicode - 'a' + 10);


you can use the public struct NSDecimal to keep track of this.

Would be longer code, and we don't need NSDecimal's greater precision, since we only need exactly as much precision as would fit in a double's mantissa.

aballway · 2017-04-20T16:22:12Z

Frameworks/Foundation/NSScanner.mm

+    if (result) {
+        *result = static_cast<float>(doubleValue);
+    }
+    return YES;
 }

 /**
 @Status Interoperable
 @Notes
 */
 - (BOOL)scanUnsignedLongLong:(unsigned long long*)pValue {


super nit: no implicit int

unsigned and long are just modifiers, not actual types, so saying unsigned long long makes the compiler infer the type which by default is int. It's common to elide the int, but I'm not a fan 🐋

ah, that. we as well as the docs forgo the int, so I'm leaving as is.

aballway · 2017-04-20T16:26:46Z

Frameworks/Foundation/NSScanner.mm

            } else {
-                *valuep = long_long_MIN;
+                *valuep = std::numeric_limits<long long>::min();


should this be lowest()?

aballway · 2017-04-20T16:30:20Z

tests/unittests/Foundation/NSScannerTests.mm

+}
+
+TEST(NSScanner, ScanLongLong) {
+    testScanSignedIntegral<long long>(@selector(scanLongLong:));


nit: they're everywhere 😿

aballway · 2017-04-20T16:31:01Z

include/Foundation/NSScanner.h

 - (BOOL)scanHexInt:(unsigned int*)intValue;
 - (BOOL)scanHexLongLong:(unsigned long long*)result;
 - (BOOL)scanInteger:(NSInteger*)value;
 - (BOOL)scanInt:(int*)intValue;
 - (BOOL)scanLongLong:(long long*)longLongValue;
- (BOOL)scanString:(NSString*)string intoString:(NSString* _Nullable*)stringValue;
+- (BOOL)scanString:(NSString* _Nonnull)string intoString:(NSString* _Nullable*)stringValue;
 - (BOOL)scanUnsignedLongLong:(unsigned long long*)unsignedLongLongValue;


nit: they're everywhere 😿

rajsesh · 2017-04-20T21:29:54Z

Frameworks/Foundation/NSScanner.mm

@@ -185,45 +150,43 @@ - (BOOL)scanInteger:(int*)valuep {
        }
    }

-    return TRUE;
+    return YES;
 }

 /**
 @Status Interoperable
 */
 - (BOOL)scanLongLong:(long long*)valuep {


ugh. I am pretty sure this works, but why not use strtoll here?

was not sure how we felt about swapping out large chunks of the previous code. there are also unichar -> char conversion implications I haven't thought about yet, but it should be fine I think...

rajsesh · 2017-04-20T21:30:57Z

Frameworks/Foundation/NSScanner.mm

+    std::vector<char> charsToScan;
+    charsToScan.reserve(_string.length - startLocation);
+    for (NSUInteger i = startLocation; i < length; ++i, ++pScan) {
+        if ((*pScan >= '0') && (*pScan <= '9')) {


(*pScan >= '0') && (*pScan <= '9' [](start = 12, length = 33)

isdigit?

rajsesh · 2017-04-20T21:32:18Z

Frameworks/Foundation/NSScanner.mm

-            continue;
-        } else {
-            return FALSE;
+    if (_charactersToBeSkipped) {


_charactersToBeSkipped [](start = 8, length = 22)

This method is going to be particularly slow.

The default case (skip whitespace, string is space separated) has low n, so it might not be as bad as you think. That said, is there an improvement you'd like to suggest? (Fetching more of the string at once?)

rajsesh · 2017-04-20T21:43:59Z

Frameworks/Foundation/NSScanner.mm

 }

 /**
- @Status Stub
+ @Status Interoperable
 @Notes
 */
 - (BOOL)scanHexDouble:(double*)result {


scanHexDouble [](start = 8, length = 13)

could sscanf_s be used for the heavylifting here?

sscanf doesn't return where it stopped scanning. you can sort of work around that by specifying "%a%s" as the format, but that scans the whole rest of the string unnecessarily... you could also do a bunch of preprocessing to give it a prepped state, but then it just ends up being the same, but with sscanf instead of ldexp

bviglietta · 2017-04-20T23:26:11Z

(BOOL)scanInteger:(int*)valuep {

NSInteger

Refers to: Frameworks/Foundation/NSScanner.mm:137 in 658f544. [](commit_id = 658f544, deletion_comment = False)

bviglietta · 2017-04-20T23:26:47Z

Frameworks/Foundation/NSScanner.mm

@@ -174,7 +139,7 @@ - (BOOL)scanInteger:(int*)valuep {

    // This assumes sizeof(long long) >= sizeof(int).
    if (![self scanLongLong:&scanValue]) {
-        return FALSE;
+        return NO;
    } else if (valuep) {
        if (scanValue > LONG_MAX) {


LONG_MAX [](start = 24, length = 8)

NSIntegerMax, etc.

bviglietta · 2017-04-20T23:31:03Z

Frameworks/Foundation/NSScanner.mm


-    pScanStart += _location;
+    char* pScan = (char*)[_string UTF8String] + startLocation;


(char*)[_string UTF8String] + startLocation [](start = 18, length = 43)

This math won't work out well if there are non-ASCII characters in the string.

bviglietta · 2017-04-20T23:36:14Z

Frameworks/Foundation/NSScanner.mm

+    // copy a scannable range of the string to a new buffer, adjusting for these constraints
+    char decimalSeparator;
+    if ((!_locale) ||
+        (![[_locale objectForKey:NSLocaleDecimalSeparator] getCString:&decimalSeparator maxLength:1 encoding:NSASCIIStringEncoding])) {


NSASCIIStringEncoding [](start = 109, length = 21)

In some locales the decimal separator won't be representable as an ASCII character.

rajsesh · 2017-04-24T18:40:23Z

Frameworks/Foundation/NSScanner.mm

+
+// Helper function for implementing a scan function for a numeric type using a CRT function such as wcstoll
+template <typename TNumeric>
+static BOOL __scanNumeric(NSScanner* self,


super nit: this could really be c++ bool.

also, scanner instead of self?

rajsesh · 2017-04-24T18:46:54Z

Frameworks/Foundation/NSScanner.mm

+template <typename TNumeric>
+static BOOL __scanNumeric(NSScanner* self,
+                          TNumeric* valuep,
+                          TNumeric (*localeFunc)(const wchar_t*, wchar_t**, int, _locale_t), // eg: _wcstoll_l


nit: you don't need a locale function here, you could use lambda to bind the locale when you need it (at the calling site).

rajsesh · 2017-04-24T18:48:16Z

Frameworks/Foundation/NSScanner.mm

-    _locale = nil;
+// Overload of __scanNumeric for CRT functions that do not take a base parameter
+template <typename TNumeric>
+static BOOL __scanNumeric(NSScanner* self,


nit: lambdas to bind base when needed?

rajsesh · 2017-04-24T18:53:35Z

Frameworks/Foundation/NSScanner.mm

-            result = NO;
-            break;
+    const NSUInteger length = _string.length;
+    NSUInteger i = [self _indexOfNextUnskippedCharacter];


nit: this could go inside the initializer part for the for statement

it could in this instance, but there are other instances in the file where i needs to be available outside the for loop and I'd rather keep them in line

Understood. You can ignore any and all the nit comments.

rajsesh · 2017-04-24T18:55:29Z

Frameworks/Foundation/NSScanner.mm

-    range.length = [string length];
-    int oldLocation = _location;
+- (BOOL)scanString:(NSString* _Nonnull)string intoString:(NSString* _Nullable*)stringp {
+    if (string.length == 0) {


the method uses string.length, and [string length], would be good to stick to one convention. also, may even make sense to stick it into a constant.

rajsesh · 2017-04-24T18:57:54Z

Frameworks/Foundation/NSScanner.mm

+                    state = STATE_P;
+
+                } else {
+                    goto DONE_SCANNING_HEXDOUBLE;


instead of the goto here, please set a flag (doneScanningDouble) and outside the switch, check for the flag and break out of the for loop. We don't like goto's for a reason and this is easy to not use it here.

rajsesh · 2017-04-24T18:59:29Z

Frameworks/Foundation/NSScanner.mm

+    double mantissaDigitPlace = 1;
+
+    const NSUInteger length = _string.length;
+    NSUInteger i = [self _indexOfNextUnskippedCharacter];


nit, can be for initializer.

rajsesh · 2017-04-24T19:00:38Z

Frameworks/Foundation/NSScanner.mm

+    const NSUInteger length = _string.length;
+    NSUInteger i = [self _indexOfNextUnskippedCharacter];
+    for (; i < length; ++i) {
+        unichar unicode = _stringChars[i];


nit: ch or even c is more appropriate than unicode.

rajsesh · 2017-04-24T19:06:41Z

Frameworks/Foundation/NSScanner.mm

+                        goto DONE_SCANNING_HEXDOUBLE; // Stop scanning once a second period has been scanned
+                    }
+
+                } else if (unicode >= '0' && unicode <= '9') {


you could extract the value of the current character first for digits and hex first and then do the scannedPeriod check. Better yet, you could have a multiplier value which is 16 before you scan period and 1/16 after you scannedPeriod, then you don't need the if checks at all. also, isdigit instead of actual comparison here?

leaving the comparison since it better contextualizes the arithmetic and is more in line with the other checks

if checks would still be necessary even using that multiplier value, since the mantissa advances in different directions before/after the period ie:

0x12, scan 3 = 0x12 * 16 + 0x3 = 0x123 (multiplier applies to previous number, is constant)
0x1.2, scan 3 = 0x1.2 + 0x3 / 16^2 = 0x1.23 (multiplier applies to new digit, changes)

I could combine scanned period and the multiplier into one var, but I think that would hurt readability.

rajsesh · 2017-04-24T19:07:30Z

Frameworks/Foundation/NSScanner.mm

+
+        switch (state) {
+            case STATE_START:
+                if (unicode == '-') {


If these are meant to be UTF16 unichars, then need L'-' etc in front of the chars. Not sure if that is automatic.

msft-Jeyaram · 2017-04-24T22:06:26Z

Frameworks/Foundation/NSScanner.mm

-    return _isCaseSensitive;
+ (instancetype)localizedScannerWithString:(NSString* _Nonnull)string {
+    NSScanner* ret = [self scannerWithString:string];
+    ret.locale = [NSLocale currentLocale];


nit: do it via an init,

there is no equivalent public init, seems like unnecessary frills to make one just for this

msft-Jeyaram · 2017-04-24T22:08:35Z

Frameworks/Foundation/NSScanner.mm

+                    state = STATE_SIGN;
+                    mantissaIsNegative = true;
+                } else if (unicode == '+') {
+                    state = STATE_SIGN;


state = STATE_SIGN; [](start = 12, length = 27)

you could just use the NSDecimal struct and if the precision is not too high, it would only take up the first slot of the mantissa array.
Also it would make the code much simpler, with tracking all the states.

hmm maybe not, given this is being looped over.

In reply to: 113068748 [](ancestors = 113068748)

msft-Jeyaram · 2017-04-24T22:09:53Z

Frameworks/Foundation/NSScanner.mm

+            case STATE_ZERO:
+                if (unicode == 'x' || unicode == 'X') {
+                    state = STATE_MANTISSA;
+                } else {


else [](start = 18, length = 4)

nit: can just return NO

? the top if case would fall through...

msft-Jeyaram · 2017-04-24T22:10:16Z

Frameworks/Foundation/NSScanner.mm

+
+            case STATE_P:
+                if (!hasValue) {
+                    goto DONE_SCANNING_HEXDOUBLE;


goto [](start = 20, length = 4)

goto :(

ms-jihua · 2017-04-26T23:50:05Z

Ping

aballway · 2017-04-27T17:19:46Z

Frameworks/Foundation/NSScanner.mm

+
+// Helper function for implementing a scan function for a numeric type using a CRT function such as wcstoll
+template <typename TNumeric>
+static BOOL __scanNumeric(NSScanner* scanner,


nit: make templated on lambda so we don't have to use std::function, and then just inline the lambdas into the __scanNumeric calls

would rather have it be strongly-typed - if it turns out we are bottlenecked by std::function overhead, we can make this change then

DHowett-MSFT · 2017-04-26T21:17:56Z

Frameworks/Foundation/NSScanner.mm

+    std::function<long(const wchar_t*, wchar_t**)> scanFunc = [self](const wchar_t* scanStart, wchar_t** scanEnd) {
+        return _crtLocale ? _wcstol_l(scanStart, scanEnd, 10, _crtLocale) : wcstol(scanStart, scanEnd, 10);
+    };
+    return __scanNumeric(self, reinterpret_cast<long*>(valuep), scanFunc);


this may not be a valid cast on all platforms. prefer a temporary that you copy into NSInteger for truncation purposes

- Implemented the following functions/properties: - locale - copyWithZone: - localizedScannerWithString: - scanDecimal: - scanHexDouble: - scanHexFloat: - Added unit tests for NSScanner (there were none, including for previously implemented features) - Fixed a number of edge cases that were incorrect Fixes microsoft#2083

…g CRT functions - Changed NSScanner to prefetch all characters in the string at init - Added tests that check for differing behaviors between 32-bit and 64-bit types - Misc CR feedback

- Renamed the states in scanHexDouble to reflect the current expected state rather than previous state

msftclas added the cla-not-required label Apr 20, 2017

ms-jihua commented Apr 20, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 20, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 20, 2017

View reviewed changes

aballway reviewed Apr 20, 2017

View reviewed changes

rajsesh reviewed Apr 20, 2017

View reviewed changes

bviglietta reviewed Apr 20, 2017

View reviewed changes

ms-jihua force-pushed the nsscanner_stubs2 branch from 658f544 to 03989dc Compare April 22, 2017 00:02

rajsesh suggested changes Apr 24, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 24, 2017

View reviewed changes

ms-jihua force-pushed the nsscanner_stubs2 branch from 03989dc to 4b278f6 Compare April 24, 2017 23:46

rajsesh approved these changes Apr 26, 2017

View reviewed changes

aballway approved these changes Apr 27, 2017

View reviewed changes

DHowett-MSFT approved these changes Apr 27, 2017

View reviewed changes

ms-jihua added 4 commits April 27, 2017 10:49

- Rewrote most NSScanner scan<Numeric> or <HexNumeric> functions usin…

3200f11

…g CRT functions - Changed NSScanner to prefetch all characters in the string at init - Added tests that check for differing behaviors between 32-bit and 64-bit types - Misc CR feedback

- CR feedback

b8cb340

- Renamed the states in scanHexDouble to reflect the current expected state rather than previous state

rebased, made NSInteger safer

5c9b489

ms-jihua force-pushed the nsscanner_stubs2 branch from 4b278f6 to 5c9b489 Compare April 27, 2017 18:00

ms-jihua merged commit dcdc18d into microsoft:develop Apr 27, 2017


		static const double c_floatingPtTolerance = 0.00001;

		// Helper template function that calls a scan_______ function specified by the selector and does the nececssary casts


		pScanStart += _location;
		char* pScan = (char*)[_string UTF8String] + startLocation;

Complete implementation of NSScanner #2582

Complete implementation of NSScanner #2582

Conversation

ms-jihua commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bviglietta commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment