Make dart:core strong-mode clean.

Make dart:core strong-mode clean.

R=leafp@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/84ec63e9fc404a6caafc53dbdf75e45907301818

[floitsch, 2016-04-16 00:28:57]

floitsch@google.com changed reviewers:
+ leafp@google.com, lrn@google.com

[floitsch, 2016-04-16 00:28:57]

I also made the 'replace' function accept a Map<String, dynamic> to align it
with the Uri constructor.

[floitsch, 2016-04-16 00:29:13]

Will add a test for the `replace` function before committing.

[Lasse Reichstein Nielsen, 2016-04-16 09:58:22]

lgtm

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/iterable.dart#...
sdk/lib/core/iterable.dart:615: _generator = (generator != null) ? generator :
_id as _Generator<E>;
That might need some documentation since the cast is known to fail if E isn't
int (or compatible). It's basically a check, not a cast.

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/num.dart
File sdk/lib/core/num.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/num.dart#newco...
sdk/lib/core/num.dart:450: /** Helper function for [parse]. */
functions

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:151: * use an empty list for `queryParameters`.
empty map?

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:2370: // TODO(ajohnsen): Consider using Uint8List.
Do consider doing that.

[floitsch, 2016-04-19 15:36:40]

Updated the Changelog too.

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/iterable.dart
File sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/iterable.dart#...
sdk/lib/core/iterable.dart:615: _generator = (generator != null) ? generator :
_id as _Generator<E>;
On 2016/04/16 09:58:22, Lasse Reichstein Nielsen wrote:
> That might need some documentation since the cast is known to fail if E isn't
> int (or compatible). It's basically a check, not a cast.

Done.

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/num.dart
File sdk/lib/core/num.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/num.dart#newco...
sdk/lib/core/num.dart:450: /** Helper function for [parse]. */
On 2016/04/16 09:58:22, Lasse Reichstein Nielsen wrote:
> functions

Done.

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:151: * use an empty list for `queryParameters`.
On 2016/04/16 09:58:22, Lasse Reichstein Nielsen wrote:
> empty map?

Done.

https://codereview.chromium.org/1895473004/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:2370: // TODO(ajohnsen): Consider using Uint8List.
On 2016/04/16 09:58:22, Lasse Reichstein Nielsen wrote:
> Do consider doing that.

Done.

[kevmoo, 2016-04-19 22:53:15]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2016-04-19 22:53:16]

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `Uri.parseIPv6Address` returns a `Uint8List`.
Where's the signature change for this update? What's the upside for users?

Keeping the signature more general seems like a good idea – unless we think
folks will be using the UInt8List-ness of this value

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:168: Map<String, dynamic/*String|Iterable<String>*/>
queryParameters,
Why this change?

[Leaf, 2016-04-19 23:58:36]

lgtm

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart
File sdk/lib/core/num.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart#n...
sdk/lib/core/num.dart:451: static int _returnIntNull(String _) => null;
The String annotation should not be required.  As an aside, this is one of those
cases where being able to "partially apply" a generic function would be useful. 
I think we actually would currently instantiate this for you if you just made
your original _returnNull a generic returning T.

I presume that the inline closure is avoided because new closures will have to
be allocated every time in order to satisfy the Dart identity semantics?

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:1399: (String key, dynamic/*String|Iterable<String>*/
value) {
I don't think these type annotations are required by strong mode are they? 
Downward inference should propagate the types since queryParameters has a type.

[floitsch, 2016-04-20 14:15:06]

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `Uri.parseIPv6Address` returns a `Uint8List`.
On 2016/04/19 22:53:16, kevmoo wrote:
> Where's the signature change for this update?
What do you mean with 'signature change' ?
It's an implementation detail: we still return a 'List<int>', but it's a
Uint8List now.

> What's the upside for users?
Less memory. More efficiency.
> 
> Keeping the signature more general seems like a good idea –&nbsp;unless we
think
> folks will be using the UInt8List-ness of this value

The signature hasn't changed.

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart
File sdk/lib/core/num.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart#n...
sdk/lib/core/num.dart:451: static int _returnIntNull(String _) => null;
On 2016/04/19 23:58:36, Leaf wrote:
> The String annotation should not be required.
But that's what the input should be. So I prefer keeping them.

> As an aside, this is one of those
> cases where being able to "partially apply" a generic function would be
useful. 
> I think we actually would currently instantiate this for you if you just made
> your original _returnNull a generic returning T.
I think this is a good case where you wouldn't want that to happen, since you
would have to reallocate a new tear-off every time you want to use the function.
Even worse, I would expect the generated code to be much less efficient.

> 
> I presume that the inline closure is avoided because new closures will have to
> be allocated every time in order to satisfy the Dart identity semantics?
correct. (something I would like to make unspecified)

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:168: Map<String, dynamic/*String|Iterable<String>*/>
queryParameters,
On 2016/04/19 22:53:16, kevmoo wrote:
> Why this change?

Because that's what the types are. We don't have union types, but adding them as
a comment may help the understanding of the code.

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:1399: (String key, dynamic/*String|Iterable<String>*/
value) {
On 2016/04/19 23:58:36, Leaf wrote:
> I don't think these type annotations are required by strong mode are they? 
> Downward inference should propagate the types since queryParameters has a
type.

Done.

[kevmoo, 2016-04-20 14:34:52]

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `Uri.parseIPv6Address` returns a `Uint8List`.
On 2016/04/20 14:15:05, floitsch wrote:
> On 2016/04/19 22:53:16, kevmoo wrote:
> > Where's the signature change for this update?
> What do you mean with 'signature change' ?
> It's an implementation detail: we still return a 'List<int>', but it's a
> Uint8List now.
> 
> > What's the upside for users?
> Less memory. More efficiency.
> > 
> > Keeping the signature more general seems like a good idea –&nbsp;unless we
> think
> > folks will be using the UInt8List-ness of this value
> 
> The signature hasn't changed.

Oh. Not needed in the changelog, then. I think it introduces more confusion than
it's worth.

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:168: Map<String, dynamic/*String|Iterable<String>*/>
queryParameters,
On 2016/04/20 14:15:06, floitsch wrote:
> On 2016/04/19 22:53:16, kevmoo wrote:
> > Why this change?
> 
> Because that's what the types are. We don't have union types, but adding them
as
> a comment may help the understanding of the code.

Oh – because we allow "foo" to have 3 values – gotcha.

[Leaf, 2016-04-20 17:48:37]

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart
File sdk/lib/core/num.dart (right):

https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/num.dart#n...
sdk/lib/core/num.dart:451: static int _returnIntNull(String _) => null;
On 2016/04/20 14:15:05, floitsch wrote:
> On 2016/04/19 23:58:36, Leaf wrote:
> > The String annotation should not be required.
> But that's what the input should be. So I prefer keeping them.

ok.

> > As an aside, this is one of those
> > cases where being able to "partially apply" a generic function would be
> useful. 
> > I think we actually would currently instantiate this for you if you just
made
> > your original _returnNull a generic returning T.
> I think this is a good case where you wouldn't want that to happen, since you
> would have to reallocate a new tear-off every time you want to use the
function.
> Even worse, I would expect the generated code to be much less efficient.

I'd hope it would be inlined, but it isn't a slam dunk.  We should make sure
that the semantics admits inlining these, if we allow them.

> > I presume that the inline closure is avoided because new closures will have
to
> > be allocated every time in order to satisfy the Dart identity semantics?
> correct. (something I would like to make unspecified)

+1

[floitsch, 2016-04-20 18:20:27]

On 2016/04/20 14:34:52, kevmoo wrote:
> https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md
> File CHANGELOG.md (right):
> 
> https://codereview.chromium.org/1895473004/diff/20001/CHANGELOG.md#newcode6
> CHANGELOG.md:6: * `Uri.parseIPv6Address` returns a `Uint8List`.
> On 2016/04/20 14:15:05, floitsch wrote:
> > On 2016/04/19 22:53:16, kevmoo wrote:
> > > Where's the signature change for this update?
> > What do you mean with 'signature change' ?
> > It's an implementation detail: we still return a 'List<int>', but it's a
> > Uint8List now.
> > 
> > > What's the upside for users?
> > Less memory. More efficiency.
> > > 
> > > Keeping the signature more general seems like a good idea –&nbsp;unless we
> > think
> > > folks will be using the UInt8List-ness of this value
> > 
> > The signature hasn't changed.
> 
> Oh. Not needed in the changelog, then. I think it introduces more confusion
than
> it's worth.
I know you had to push us to write Changelog entries, but it worked. I actually
changed my mind, and I would prefer if we wrote more entries. In particular we
should write all changes that have a user-visible effect.
In this case the effect is very minor, but it can be seen, and as such I would
prefer writing it.
If you think it's counter-productive I can remove it, though.
> 
> https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart
> File sdk/lib/core/uri.dart (right):
> 
>
https://codereview.chromium.org/1895473004/diff/20001/sdk/lib/core/uri.dart#n...
> sdk/lib/core/uri.dart:168: Map<String, dynamic/*String|Iterable<String>*/>
> queryParameters,
> On 2016/04/20 14:15:06, floitsch wrote:
> > On 2016/04/19 22:53:16, kevmoo wrote:
> > > Why this change?
> > 
> > Because that's what the types are. We don't have union types, but adding
them
> as
> > a comment may help the understanding of the code.
> 
> Oh –&nbsp;because we allow "foo" to have 3 values –&nbsp;gotcha.

[floitsch, 2016-04-21 12:01:40]

@Kevin: landing as is, but will do a follow-up CL if you think that I shouldn't
change the Changelog.

[floitsch, 2016-04-21 12:01:53]

Description was changed from

==========
Make dart:core strong-mode clean.
==========

to

==========
Make dart:core strong-mode clean.

R=leafp@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/84ec63e9fc404a6caafc53dbdf75e45907301818
==========

[floitsch, 2016-04-21 12:01:54]

Committed patchset #3 (id:40001) manually as
84ec63e9fc404a6caafc53dbdf75e45907301818 (presubmit successful).