Allow dartk to print out binary Kernel IR results to stdout if requested.

Allow dartk to print out binary Kernel IR results to stdout if requested.

BUG=
R=johnniwinther@google.com

Committed: https://github.com/dart-lang/sdk/commit/6b398ef91cd3458b1cdfeb6b6631c7089af26193

[Emily Fortuna, 2017-03-31 01:07:38]

efortuna@google.com changed reviewers:
+ asgerf@google.com, johnniwinther@google.com

[Emily Fortuna, 2017-03-31 01:07:39]

This is potentially a controversial change... the reason I want to do this is I
want to write some tests that call dartk, and then do something with the
(binary) output. As it was, if you ran dartk.dart -fbin it would output to
out.dill, and if you tried to specify to stdout by passing -onull it would
create a file called null.

[Emily Fortuna, 2017-03-31 01:08:04]

Patchset #1 (id:1) has been deleted

[Johnni Winther, 2017-03-31 06:38:35]

LGTM but wait for Asger's approval.

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.dart
File pkg/kernel/lib/kernel.dart (right):

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:72: /// Silly wrapper around BytesBuilder that
conforms to the Sink interface.
'Silly' -> 'Simple'

[asgerf, 2017-03-31 08:40:16]

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.dart
File pkg/kernel/lib/kernel.dart (right):

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:34: sink = new _ByteBuffer();
Is the wrapper necessary?  Could you try this instead?

var sink = stdout.nonBlocking
new BinaryPrinter(sink).writeProgramFile(program)
return sink.flush()

(i.e with stdout.nonBlocking as the sink, but without closing it)

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:46: if (sink is _ByteBuffer) print(sink.takeBytes());
Calling toString on a List<int> normally emits something of form [1, 2, 3, ...].
Is this the intention? I think it would make more sense to print the actual byte
stream.

(With the change I suggested above we will not need the print)

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:79: _builder = new BytesBuilder();
You can pass `copy: false` to the BytesBuilder here.

[Emily Fortuna, 2017-03-31 17:46:56]

Thank you for the recommendation, Asger, that's much better! I'm happy to add a
follow-up CL if you have additional recommendations.

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.dart
File pkg/kernel/lib/kernel.dart (right):

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:34: sink = new _ByteBuffer();
On 2017/03/31 08:40:16, asgerf wrote:
> Is the wrapper necessary?  Could you try this instead?
> 
> var sink = stdout.nonBlocking
> new BinaryPrinter(sink).writeProgramFile(program)
> return sink.flush()
> 
> (i.e with stdout.nonBlocking as the sink, but without closing it)

Ah, that's MUCH better. I didn't know about that. Thanks!

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:46: if (sink is _ByteBuffer) print(sink.takeBytes());
On 2017/03/31 08:40:16, asgerf wrote:
> Calling toString on a List<int> normally emits something of form [1, 2, 3,
...].
> Is this the intention? I think it would make more sense to print the actual
byte
> stream.
> 
> (With the change I suggested above we will not need the print)

touche. No I hadn't actually tested consuming this input, so I bet I was about
to run into this problem.

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:72: /// Silly wrapper around BytesBuilder that
conforms to the Sink interface.
On 2017/03/31 06:38:35, Johnni Winther wrote:
> 'Silly' -> 'Simple'

Acknowledged.

https://codereview.chromium.org/2784303003/diff/20001/pkg/kernel/lib/kernel.d...
pkg/kernel/lib/kernel.dart:79: _builder = new BytesBuilder();
On 2017/03/31 08:40:16, asgerf wrote:
> You can pass `copy: false` to the BytesBuilder here.

No longer needed once I took this out.

[Emily Fortuna, 2017-03-31 17:48:14]

Description was changed from

==========
Allow dartk to print out binary Kernel IR results to stdout if requested.

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

to

==========
Allow dartk to print out binary Kernel IR results to stdout if requested.

BUG=
R=johnniwinther@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6b398ef91cd3458b1cdfeb6b6631c7089af26193
==========

[Emily Fortuna, 2017-03-31 17:48:15]

Committed patchset #2 (id:40001) manually as
6b398ef91cd3458b1cdfeb6b6631c7089af26193 (presubmit successful).