adding codereview file, formatting, adding gitignore

adding codereview file, formatting, adding gitignore

R=lrn@google.com

Committed: https://github.com/dart-lang/isolate/commit/deff8268aac68f7cbfd59b45b9b04299f5609d2c

[kevmoo, 2015-02-25 16:26:28]

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

[Lasse Reichstein Nielsen, 2015-02-26 10:59:16]

OK to the addition and some of the formatting, but also no to a large part of
the formatting changes.

https://codereview.chromium.org/955053002/diff/1/example/http-server.dart
File example/http-server.dart (right):

https://codereview.chromium.org/955053002/diff/1/example/http-server.dart#new...
example/http-server.dart:20: ..[1] = listener).then((SendPort stopPort) => () =>
_sendStop(stopPort));
No to this change. It is not more readable.

https://codereview.chromium.org/955053002/diff/1/example/http-server.dart#new...
example/http-server.dart:119: });
Also no to this change.

Putting the ".then" after a "})" makes it less visible.
I admit that the indentation could be smaller, but I'd prefer to put the
Future.wait result into a variable instead.

https://codereview.chromium.org/955053002/diff/1/example/runner-pool.dart
File example/runner-pool.dart (right):

https://codereview.chromium.org/955053002/diff/1/example/runner-pool.dart#new...
example/runner-pool.dart:35: List<Future> fibs = new List(limit + 1);
I really dislike having the body indented less than the previous line if it can
be avoided.

https://codereview.chromium.org/955053002/diff/1/lib/isolaterunner.dart
File lib/isolaterunner.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/isolaterunner.dart#newco...
lib/isolaterunner.dart:63: (Isolate isolate) {
Also has body less indented than its delimiter line. Indent the body by at least
four more.

https://codereview.chromium.org/955053002/diff/1/lib/isolaterunner.dart#newco...
lib/isolaterunner.dart:141: callback: _kTrue, timeout: timeout, onTimeout:
_kFalse);
I prefer the original.

https://codereview.chromium.org/955053002/diff/1/lib/isolaterunner.dart#newco...
lib/isolaterunner.dart:248: });
Less consistently indented. I'd prefer the onListen: and onCancel: to be
indented at the same level.

https://codereview.chromium.org/955053002/diff/1/lib/isolaterunner.dart#newco...
lib/isolaterunner.dart:310: new Future.sync(() => function(argument)),
responsePort);
It's not as visible that responsePort is a second argument to sendFutureResult,
and not an argument to Future.sync.
I prefer the original.

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart
File lib/loadbalancer.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcode61
lib/loadbalancer.dart:61: cleanUp: (Runner runner) {
indent cleanUp to after the '('. Putting the body on three lines seems
unnecessary.

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcode85
lib/loadbalancer.dart:85: {Duration timeout, onTimeout(), int load: 100}) {
Indent "{" to after then "(". That is:
Future run(function...,
           {Duration timeout, ... }) {

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcod...
lib/loadbalancer.dart:109: {Duration timeout, onTimeout(), int load: 100}) {
Indent '{ ' to after '('.

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcod...
lib/loadbalancer.dart:114: load: load, timeout: timeout, onTimeout: onTimeout));
Indent arguments to after '(' or put all arguments on new lines.

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcod...
lib/loadbalancer.dart:185: int childIndex = index * 2 + 1; // Left child index.
There should always be two spaces before an end-of-line comment on a code line.

https://codereview.chromium.org/955053002/diff/1/lib/loadbalancer.dart#newcod...
lib/loadbalancer.dart:295: balancer._decreaseLoad(this, load);
Indent body by four more.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart
File lib/ports.dart (left):

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#oldcode255
lib/ports.dart:255: 
I'm not sure I always want single lines between functions, some times two lines
are deliberate. Here it's probably just inconsistent.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart
File lib/ports.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#newcode49
lib/ports.dart:49: {Duration timeout, var timeoutValue}) {
Indent '{' to after '('.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#newcode96
lib/ports.dart:96: {callback(message), Duration timeout, onTimeout()}) {
Indent '{' to after '(' or put the completer on a new line as well.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#newcode156
lib/ports.dart:156: {Duration timeout, var timeoutValue}) {
Indentation again.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#newcode223
lib/ports.dart:223: {Duration timeout, onTimeout()}) {
Indent '{'.

https://codereview.chromium.org/955053002/diff/1/lib/ports.dart#newcode226
lib/ports.dart:226: callback: receiveFutureResult, timeout: timeout, onTimeout:
onTimeout);
Put completer on a new line too, or indent as original.

https://codereview.chromium.org/955053002/diff/1/lib/registry.dart
File lib/registry.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/registry.dart#newcode77
lib/registry.dart:77: {Duration timeout: const Duration(seconds: 5)})
Indent '{' to after '('.

https://codereview.chromium.org/955053002/diff/1/lib/registry.dart#newcode401
lib/registry.dart:401: if (entry == null) continue; // Entry was removed.
Two spaces before '//'.

https://codereview.chromium.org/955053002/diff/1/lib/registry.dart#newcode416
lib/registry.dart:416: if (entry == null) continue; // Object was removed.
Ditto.

https://codereview.chromium.org/955053002/diff/1/lib/registry.dart#newcode470
lib/registry.dart:470: if (max == null) max = matchingIds.length; // All
results.
And ditto on the spaces.

https://codereview.chromium.org/955053002/diff/1/lib/runner.dart
File lib/runner.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/runner.dart#newcode45
lib/runner.dart:45: {Duration timeout, onTimeout()}) {
Indent as original.

https://codereview.chromium.org/955053002/diff/1/lib/src/errors.dart
File lib/src/errors.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/src/errors.dart#newcode50
lib/src/errors.dart:50: {cleanUp(successResult)}) {
Indent as original.

https://codereview.chromium.org/955053002/diff/1/lib/src/functionref.dart
File lib/src/functionref.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/src/functionref.dart#new...
lib/src/functionref.dart:71: "Function $functionName${classQualifier} not found
in library $libraryUri");
Long line. The original was badly indented, but I'm not sure this is better.

https://codereview.chromium.org/955053002/diff/1/lib/src/lists.dart
File lib/src/lists.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/src/lists.dart#newcode35
lib/src/lists.dart:35: ..[4] = v5;
No to all changes here.
maybe if the "new List(n)" was moved to a new line too, it would look better:

List .... =>
    new List(4)
      ..[0] = v1
      ..[1] = v2;
but otherwise the original was more readable because it was clear what the ".."
applied to.

https://codereview.chromium.org/955053002/diff/1/lib/src/multiplexport.dart
File lib/src/multiplexport.dart (right):

https://codereview.chromium.org/955053002/diff/1/lib/src/multiplexport.dart#n...
lib/src/multiplexport.dart:94: if (receivePort == null) return; // Port closed.
Two spaces before //.

https://codereview.chromium.org/955053002/diff/1/test/isolaterunner_test.dart
File test/isolaterunner_test.dart (right):

https://codereview.chromium.org/955053002/diff/1/test/isolaterunner_test.dart...
test/isolaterunner_test.dart:41: Future runAll(action(IsolateRunner runner, int
index)) {
Indent body by four more.

https://codereview.chromium.org/955053002/diff/1/test/isolaterunner_test.dart...
test/isolaterunner_test.dart:46: return runAll((runner, i) =>
runner.run(setGlobal, i + 1)).then((values) {
This one is acceptable, but I still think it's better when .then(values) are
indented where it's easier to see, so you can see where the "values" variable is
bound.

https://codereview.chromium.org/955053002/diff/1/test/ports_test.dart
File test/ports_test.dart (right):

https://codereview.chromium.org/955053002/diff/1/test/ports_test.dart#newcode69
test/ports_test.dart:69: timeout: MS * 100, timeoutValue: 37);
Indent to after '(' or put completer.complete on new line.

https://codereview.chromium.org/955053002/diff/1/test/ports_test.dart#newcode92
test/ports_test.dart:92: });
This is much better!

https://codereview.chromium.org/955053002/diff/1/test/registry_test.dart
File test/registry_test.dart (right):

https://codereview.chromium.org/955053002/diff/1/test/registry_test.dart#newc...
test/registry_test.dart:44: }).then((_) => registry.lookup()).then((all) {
Less clear which operations are performed. The registry.lookup is the important
call in this test, and it's hidden in the middle of a line of of other stuff.

[kevmoo, 2015-03-03 18:14:39]

I removed the formatting delta.

PTAL

[Lasse Reichstein Nielsen, 2015-03-06 13:07:24]

LGTM, thanks.

[kevmoo, 2015-03-06 16:10:27]

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