Make number formatting in Intl able to work with Int64 or other types.

Make number formatting in Intl able to work with Int64 or other types.

BUG=

[Alan Knight, 2014-12-05 23:59:14]

alanknight@google.com changed reviewers:
+ sra@google.com

[sra1, 2014-12-16 01:40:21]

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
File pkg/intl/lib/src/intl/number_format.dart (right):

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:116: var _integerPart;
If possible, keep these in locals.

Mutable fields are worse for dart2js type inference, for example, they are seen
as initialized to null.

They are also a memory leak hazard if not cleared.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:278: /** Helpers to check numbers that
don't conform to, e.g. Int64 */
Helpers to check number types that don't implement the [num] interface, e.g.
Int64.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:293: _fractionPart = 0;
_extraIntegerDigits is not set here, so does it get the value from the previous
formatting?

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:362: 
This could be

   String paddedExtra = extra;
   if (intDigits.isNotEmpty) {
     paddedExtra = extra.padLeft(_multiplierDigits, '0');
   }

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:366: // Remove the minus sign.
It would be nice if we could take care of this out of band.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:370: return digitString.codeUnits;
Might as well just return the string and make the loop that walks the digits
index with  s.codeUnitAt(i)

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:454: String _signPrefix(x) =>
x.isNegative ? _negativePrefix : _positivePrefix;
Perhaps pass the result of x.isNegative, i.e. take a bool.

[Alan Knight, 2014-12-16 19:34:08]

PTAL

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
File pkg/intl/lib/src/intl/number_format.dart (right):

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:116: var _integerPart;
On 2014/12/16 01:40:20, sra1 wrote:
> If possible, keep these in locals.
> 
> Mutable fields are worse for dart2js type inference, for example, they are
seen
> as initialized to null.
> 
> They are also a memory leak hazard if not cleared.

Done. I'd put them there originally because it was too painful passing them
around to all the different methods, some of which modified them, but the code's
changed enough since then that it wasn't too bad to pass them.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:278: /** Helpers to check numbers that
don't conform to, e.g. Int64 */
On 2014/12/16 01:40:21, sra1 wrote:
> Helpers to check number types that don't implement the [num] interface, e.g.
> Int64.

Done.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:293: _fractionPart = 0;
On 2014/12/16 01:40:20, sra1 wrote:
> _extraIntegerDigits is not set here, so does it get the value from the
previous
> formatting?

Done.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:362: 
On 2014/12/16 01:40:20, sra1 wrote:
> This could be
> 
>    String paddedExtra = extra;
>    if (intDigits.isNotEmpty) {
>      paddedExtra = extra.padLeft(_multiplierDigits, '0');
>    }

Done.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:366: // Remove the minus sign.
On 2014/12/16 01:40:20, sra1 wrote:
> It would be nice if we could take care of this out of band.

I'm not sure what you mean by out of band, but looking at this it does seem
better to do when computing intDigits on the basic number, so did that.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:370: return digitString.codeUnits;
On 2014/12/16 01:40:20, sra1 wrote:
> Might as well just return the string and make the loop that walks the digits
> index with  s.codeUnitAt(i)

Done.

https://codereview.chromium.org/778293002/diff/1/pkg/intl/lib/src/intl/number...
pkg/intl/lib/src/intl/number_format.dart:454: String _signPrefix(x) =>
x.isNegative ? _negativePrefix : _positivePrefix;
On 2014/12/16 01:40:20, sra1 wrote:
> Perhaps pass the result of x.isNegative, i.e. take a bool.

Why? It seems like it makes the caller more awkward, e.g.
   "${_signPrefix(number)}stuff"
becomes
   "${_signPrefix(number.isNegative)}stuff"

[sra1, 2014-12-16 21:57:24]

lgtm

[sra1, 2014-12-16 21:58:27]

https://codereview.chromium.org/778293002/diff/20001/pkg/intl/pubspec.yaml
File pkg/intl/pubspec.yaml (right):

https://codereview.chromium.org/778293002/diff/20001/pkg/intl/pubspec.yaml#ne...
pkg/intl/pubspec.yaml:2: version: 0.11.11
Should bump the version so users can select this new version.

[Alan Knight, 2014-12-16 22:07:51]

https://codereview.chromium.org/778293002/diff/20001/pkg/intl/pubspec.yaml
File pkg/intl/pubspec.yaml (right):

https://codereview.chromium.org/778293002/diff/20001/pkg/intl/pubspec.yaml#ne...
pkg/intl/pubspec.yaml:2: version: 0.11.11
On 2014/12/16 21:58:27, sra1 wrote:
> Should bump the version so users can select this new version.

Done.

[Alan Knight, 2014-12-16 22:27:09]

Committed in r42411