NumberFormatter: Fix the pattern default .nostyle pattern for ICU62+

[spevans]

• When using NumberFormatter with .nostyle (UNUM_PATTERN_DECIMAL) the
  default pattern '#;#' omits the '-' symbol when formatting negative
  numbers using ICU versions 62 or later.

• Use a default pattern of '#' which lets ICU automatically set the
  format for negative numbers. This pattern is the default used by
  ICU62.

• Tested on Ubuntu14.04 with ICU 52 and Ubuntu18.04 with ICU 63.1.

[spevans]

@swift-ci test

[itaiferber]

Looks reasonable, but should this be gated on #if U_ICU_VERSION_MAJOR_NUM >= 62? Also, worth adding unit tests I think.

[spevans]

I tested with older versions of ICU (52 on Ubuntu14.04) and it worked there. I was actually hoping that if apple/swift#19860 is merged soon we could make ICU62 (or some other recent version) a requirement for corelibs-foundation since a lot of the tests actually fail on older versions and they are just commented out to hide that fact. What do you think?

I will add some more tests.

[itaiferber]

That policy sounds reasonable. I'm still concerned about the necessity of this change though — I ended up checking this on a system with ICU 62 installed:

#import <Foundation/Foundation.h>
#include <unicode/unum.h>

int main()
{
    @autoreleasepool {
#define U_CHECK(status, message) if (U_FAILURE(status)) { NSLog(message @": %s", u_errorName(status)); exit(EXIT_FAILURE); }
        UErrorCode status = U_ZERO_ERROR;
        UNumberFormat *formatter = unum_open(UNUM_PATTERN_DECIMAL, u"#;#", -1, "en_US", NULL, &status);
        U_CHECK(status, @"unum_open");
        
        UChar result[BUFSIZ] = {0};
        int32_t length = unum_format(formatter, -5, result, BUFSIZ, NULL, &status);
        U_CHECK(status, @"unum_format");
        NSLog(@"ICU %d.%d: %@", U_ICU_VERSION_MAJOR_NUM, U_ICU_VERSION_MINOR_NUM, [[NSString alloc] initWithCharactersNoCopy:result length:length freeWhenDone:NO]);
    }
}

What do you see for the above? I get ICU 62.1: -5 just fine. Am I missing something? Is this a platform compatibility issue?

[spevans]

Running your test on Ubuntu18.04 gives the following:

With the system supplied ICU:
ICU 60.2: -5

With the ICU compiled as as part of the swift build:
ICU 62.1: 5

I wonder if the difference is due to local changes made in Apple's fork?

[spevans]

Here's a test program I wrote that retrieves the pattern after setting it and tests different patterns

#include <stdio.h>
#include <stdlib.h>
#include <unicode/unum.h>

#define U_CHECK(status, message) if (U_FAILURE(status)) {  printf(": %s", u_errorName(status)); exit(EXIT_FAILURE); }

void print_uchar_str(UChar *ustr) {
        while (*ustr) {
                printf("%c", (char)*ustr);
                ustr++;
        }
        puts("");
}

void show_pattern(UNumberFormat *nf) {
  UErrorCode status = U_ZERO_ERROR;
  UChar ubuff[1024] = { 0 };
  printf("Current pattern: ");
  unum_toPattern(nf, 0, ubuff, 1023, &status);
  U_CHECK(status, "unum_toPattern");
  print_uchar_str(ubuff);
}

void set_pattern(UNumberFormat *nf, char *pattern) {
  UErrorCode status = U_ZERO_ERROR;
  printf("Setting pattern to: '%s'\n", pattern);
  UChar ubuff[16];
  
  int i = 0;
  char ch = *pattern;
  while (ch != 0) {
    ubuff[i] = (UChar)ch;
    i++;
    ch = *(pattern+i);
  }
  unum_applyPattern(nf, 0, ubuff, i, NULL, &status);
  U_CHECK(status, "unum_applyPattern");
  show_pattern(nf);
}

void test_pattern(UNumberFormat *nf, char *pattern) {
  UChar ubuff[20];
  UErrorCode status = U_ZERO_ERROR;

  set_pattern(nf, pattern);
  unum_setAttribute(nf, UNUM_MAX_INTEGER_DIGITS, 42);
  unum_setAttribute(nf, UNUM_MAX_FRACTION_DIGITS, 3);
  
  unum_formatDecimal(nf, "-12345", -1, ubuff, 19, NULL, &status);
  U_CHECK(status, "unum_formatDecimal");
  printf("Result: -12345 -> ");
  print_uchar_str(ubuff);
  unum_formatDecimal(nf, "12.345", -1, ubuff, 19, NULL, &status);
  U_CHECK(status, "unum_formatDecimal");
  printf("Result: 12.345 -> ");
  print_uchar_str(ubuff);
  puts("");
}

int main() {
  UErrorCode status = U_ZERO_ERROR;
  printf("ICU %d.%d: ", U_ICU_VERSION_MAJOR_NUM, U_ICU_VERSION_MINOR_NUM);

  UNumberFormat *nf = unum_open(UNUM_PATTERN_DECIMAL, NULL, -1, NULL, NULL, &status);
  U_CHECK(status, "unum_open");
  show_pattern(nf);
  test_pattern(nf, "#;#");
  test_pattern(nf, "#");
}

Testing on Ubuntu18.04:

System ICU

ubuntu1804:~/src$ clang -Wall -O2 -o icutest  icutest.c -licui18n -licuuc -licudata
ubuntu1804:~/src $ ./icutest
ICU 60.2: Current pattern: #.####################################################################################################################################################################################################################################################################################################################################################
Setting pattern to: '#;#'
Current pattern: #
Result: -12345 -> -12345
Result: 12.345 -> 12.345

Setting pattern to: '#'
Current pattern: #
Result: -12345 -> -12345
Result: 12.345 -> 12.345

So when setting the pattern to #;# it gets converted internally to # anyway!

ICU62 compiled as part of swift

ubuntu1804:~/src $ clang -Wall -O2 -o icutest62 -I ~/swift-install/usr/include/ -L ~/swift-install/usr/lib/swift/linux  icutest.c -licui18nswift -licuucswift -licudataswift
ubuntu1804:~/src $ LD_LIBRARY_PATH=~/swift-install/usr/lib/swift/linux ./icutest62    
ICU 62.1: Current pattern: #
Setting pattern to: '#;#'
Current pattern: 0;0
Result: -12345 -> 12345
Result: 12.345 -> 12.345

Setting pattern to: '#'
Current pattern: 0
Result: -12345 -> -12345
Result: 12.345 -> 12.345

I also tested my change on HighSierra and the corelibs-foundation tests ran ok. I haven't tested on Mojave as I don't have it installed, are you able to test this PR on Mojave?

[millenomi]

Given you checked, I'm going to take this, but we should investigate why the discrepancy.

[millenomi]

@swift-ci please test and merge

[itaiferber]

After some internal discussion right now, I came to learn why this appeared unnecessary on Darwin: despite us having ICU-62, our internal fork did not take the number formatting changes there for stability at the time (so we continued getting away with "#;#"); when the system takes these changes, this will be necessary internally as well.

(For anyone interested, see 46470466)