Fix warnings and hints in the SDK

Fix warnings and hints in the SDK

BUG=
R=paulberry@google.com, sgjesse@google.com, whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45331

[kaendfinger, 2015-03-10 13:15:50]

kaendfinger@gmail.com changed reviewers:
+ paulberry@google.com, sgjesse@google.com, whesse@chromium.org

[Bill Hesse, 2015-03-10 13:33:38]

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

[Bill Hesse, 2015-03-10 13:33:38]

This looks OK to me. I'm not sure that making all return nulls explicit is
right, but in many cases it is good to be explicit, but maybe we should
distinguish between cases that are reachable and those that are supposed to be
unreachable.

LGTM, but others should also review it.

[Paul Berry, 2015-03-10 13:49:12]

lgtm

[kaendfinger, 2015-03-10 13:56:09]

On 2015/03/10 13:33:38, Bill Hesse wrote:
> This looks OK to me. I'm not sure that making all return nulls explicit is
> right, but in many cases it is good to be explicit, but maybe we should
> distinguish between cases that are reachable and those that are supposed to be
> unreachable.
> 
> LGTM, but others should also review it.

I'm going by the analyzer hints. I'm making the assumption that the analyzer
knows best.

[Søren Gjesse, 2015-03-10 14:06:21]

https://codereview.chromium.org/996683002/diff/1/sdk/lib/async/stream_impl.dart
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/996683002/diff/1/sdk/lib/async/stream_impl.da...
sdk/lib/async/stream_impl.dart:1021: return null;
Please don't add this. It gives the impression that getting here could be
intentional, but it is actually unreachable.

We should not put in code to suppress warnings.

How about:

Remove the else (not needed as the if above returns) and adding a default to the
switch with 'throw "Unreachable";'

[kaendfinger, 2015-03-10 14:11:01]

On 2015/03/10 14:06:21, Søren Gjesse wrote:
>
https://codereview.chromium.org/996683002/diff/1/sdk/lib/async/stream_impl.dart
> File sdk/lib/async/stream_impl.dart (right):
> 
>
https://codereview.chromium.org/996683002/diff/1/sdk/lib/async/stream_impl.da...
> sdk/lib/async/stream_impl.dart:1021: return null;
> Please don't add this. It gives the impression that getting here could be
> intentional, but it is actually unreachable.
> 
> We should not put in code to suppress warnings.
> 
> How about:
> 
> Remove the else (not needed as the if above returns) and adding a default to
the
> switch with 'throw "Unreachable";'

Taken.

[Søren Gjesse, 2015-03-10 14:20:52]

lgtm

[kaendfinger, 2015-04-10 22:23:58]

The CQ bit was checked by kaendfinger@gmail.com

[kaendfinger, 2015-04-10 22:23:59]

The patchset sent to the CQ was uploaded after l-g-t-m from whesse@google.com,
paulberry@google.com
Link to the patchset: https://codereview.chromium.org/996683002/#ps20001 (title:
"Take suggestions from sgjesse")

[commit-bot: I haz the power, 2015-04-10 22:26:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-10 22:26:44]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Søren Gjesse, 2015-04-22 10:35:59]

Committed patchset #2 (id:20001) manually as 45331 (presubmit successful).

[kaendfinger, 2015-04-22 12:44:37]

On 2015/04/22 10:35:59, Søren Gjesse wrote:
> Committed patchset #2 (id:20001) manually as 45331 (presubmit successful).

Did it cuase an issue?

[Søren Gjesse, 2015-04-22 12:55:09]

On 2015/04/22 12:44:37, kaendfinger wrote:
> On 2015/04/22 10:35:59, Søren Gjesse wrote:
> > Committed patchset #2 (id:20001) manually as 45331 (presubmit successful).
> 
> Did it cuase an issue?

Yes the build-bots went red. I think it is the removed import of collection in
typed_data. One or more of the patch files expected it to be there. I will land
it with some changes later.