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

sdk/lib/internal/iterable.dart

Index: sdk/lib/internal/iterable.dart
diff --git a/sdk/lib/internal/iterable.dart b/sdk/lib/internal/iterable.dart
index 7f6b8917f357d088185aa4d462b139c42d46dd43..36c39cae5c8e7b248865e50a52a688407f994bde 100644
--- a/sdk/lib/internal/iterable.dart
+++ b/sdk/lib/internal/iterable.dart
@@ -587,11 +587,15 @@ class SkipIterable<E> extends Iterable<E> {
     return new SkipIterable<E>._(iterable, count);
   }
 
-  SkipIterable._(this._iterable, this._skipCount) {
-    if (_skipCount is! int) {
-      throw new ArgumentError.value(_skipCount, "count is not an integer");
+  SkipIterable._(this._iterable, int skipCount)
+      : _skipCount = _checkSkip(skipCount);

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

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"));

[sra1, 2017/04/28 18:38:36]

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.

+
+  static int _checkSkip(int skipCount) {

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

If not inlined, maybe name this _checkCount and remove references to "skip".
It's generic enough that it can be reused.

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

Done.

+    if (skipCount is! int) {
+      throw new ArgumentError.value(skipCount, "count is not an integer");

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

Second argument is the argument name, not a message, so:

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

[sra1, 2017/04/28 18:38:36]

Acknowledged.

     }
-    RangeError.checkNotNegative(_skipCount, "count");
+    RangeError.checkNotNegative(skipCount, "count");

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

Could we combine the two:

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

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

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

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

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.

+    return skipCount;
   }
 
   Iterable<E> skip(int count) {