SkipIterable - ensure field is only initialized to a known int.

SkipIterable - ensure field is only initialized to a known int.

By checking before initializing, dart2js can infer the field is not null

BUG=
R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/0539c0b8bf38a9fa49d178021e67b36bc9c6e41e

[sra1, 2017-04-28 02:39:32]

Patchset #1 (id:1) has been deleted

[sra1, 2017-04-28 02:40:03]

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

[Lasse Reichstein Nielsen, 2017-04-28 06:25:23]

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
File sdk/lib/internal/iterable.dart (right):

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:591: : _skipCount = _checkSkip(skipCount);
How about inlining the check:

 : _skipCount = ((skipCount is int) ? skipCount
     : throw new ArgumentError.value(skipCount, "skipCount", 
           "not an integer")) {
  RangeError.checkNotNegative(_skipCount, "skipCount");
}

or combined:

 : _skipCount = ((skipCount is int && skipCount >= 0) ? skipCount
     : throw new ArgumentError.value(skipCount, "skipCount", 
           "not a non-negative integer"));

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:593: static int _checkSkip(int skipCount) {
If not inlined, maybe name this _checkCount and remove references to "skip".
It's generic enough that it can be reused.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:595: throw new ArgumentError.value(skipCount,
"count is not an integer");
Second argument is the argument name, not a message, so:

throw new ArgumentError.value(skipCount, "skipCount", "not an integer");

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:597: RangeError.checkNotNegative(skipCount,
"count");
Could we combine the two:

if (skipCount is! int || skipCount < 0) {
  throw new ArgumentError.value(skipCount, "skipCount",
      "not a non-negative integer");
}

[Lasse Reichstein Nielsen, 2017-04-28 08:51:49]

lgtm

[sra1, 2017-04-28 18:38:37]

PTAL

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
File sdk/lib/internal/iterable.dart (right):

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:591: : _skipCount = _checkSkip(skipCount);
On 2017/04/28 06:25:23, Lasse Reichstein Nielsen wrote:
> How about inlining the check:
> 
>  : _skipCount = ((skipCount is int) ? skipCount
>      : throw new ArgumentError.value(skipCount, "skipCount", 
>            "not an integer")) {
>   RangeError.checkNotNegative(_skipCount, "skipCount");
> }
> 
> or combined:
> 
>  : _skipCount = ((skipCount is int && skipCount >= 0) ? skipCount
>      : throw new ArgumentError.value(skipCount, "skipCount", 
>            "not a non-negative integer"));

The trouble with combined conditions (&&, ||) is that they are compiled as
expressions yielding a value, which makes it difficult for dart2js to understand
that all the conditions apply to the final value in the non-throw branch.

I want to fix this problem with dart2js but it is harder than is should be.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:593: static int _checkSkip(int skipCount) {
On 2017/04/28 06:25:23, Lasse Reichstein Nielsen wrote:
> If not inlined, maybe name this _checkCount and remove references to "skip".
> It's generic enough that it can be reused.

Done.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:595: throw new ArgumentError.value(skipCount,
"count is not an integer");
On 2017/04/28 06:25:23, Lasse Reichstein Nielsen wrote:
> Second argument is the argument name, not a message, so:
> 
> throw new ArgumentError.value(skipCount, "skipCount", "not an integer");

Acknowledged.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:597: RangeError.checkNotNegative(skipCount,
"count");
On 2017/04/28 06:25:23, Lasse Reichstein Nielsen wrote:
> Could we combine the two:
> 
> if (skipCount is! int || skipCount < 0) {
>   throw new ArgumentError.value(skipCount, "skipCount",
>       "not a non-negative integer");
> }

See above. dart2js does much better at understanding code that goes one if-test
at a time.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:602: if (_skipCount is! int) {
Why is this checking _skipCount instead of count?

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:606: return new SkipIterable<E>._(_iterable,
_skipCount + count);
.skip(1).skip(-1) 'works'

[Lasse Reichstein Nielsen, 2017-05-01 05:32:00]

lgtm

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
File sdk/lib/internal/iterable.dart (right):

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:597: RangeError.checkNotNegative(skipCount,
"count");
I'm a little concerned about writing code to match the quirks of a single
implementation. It works, but you improve dart2js, will we go back and fix the
code? I haven't been told about any improvements to dart2js that means I can
change things back to the more immediate style for changes we have done in the
past. And are we sure it won't make things worse for the VM?

IOW, it is, or should be, a short-term hack to fix the library, not the compiler
(because it only affects our library code, not user code), but I'm seeing it
being used as a "solution" and never turned back.

That worry aside, let's do this now, but please, make it a priority to fix the
real problem.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:602: if (_skipCount is! int) {
That looks like a bug. Good catch.

https://codereview.chromium.org/2845933002/diff/20001/sdk/lib/internal/iterab...
sdk/lib/internal/iterable.dart:606: return new SkipIterable<E>._(_iterable,
_skipCount + count);
Iterable.skip is documented as requiring a non-negative argument, so "working"
isn't a good thing, it's just a way to hide a bug.

[sra1, 2017-05-01 17:01:14]

Description was changed from

==========
SkipIterable - ensure field is only initialized to a known int.

By checking before initializing, dart2js can infer the field is not null

BUG=
==========

to

==========
SkipIterable - ensure field is only initialized to a known int.

By checking before initializing, dart2js can infer the field is not null

BUG=
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0539c0b8bf38a9fa49d178021e67b36bc9c6e41e
==========

[sra1, 2017-05-01 17:01:15]

Committed patchset #2 (id:40001) manually as
0539c0b8bf38a9fa49d178021e67b36bc9c6e41e (presubmit successful).