Make more JS files beter match the coding standard.

Make more JS files better match the coding standard.

This includes adding missing semicolons and braces, removing superfluous semicolons and breaking long lines.

[Lasse Reichstein, 2011-12-08 08:44:16]

Have fun :)

[Rico, 2011-12-08 10:24:37]

LGTM
I see 4 or 5 files that are empty here (both before and after view), e.g.,
mjsunit/substr.js. These are not empty in my checkout, please make sure to not
delete those.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
File test/mjsunit/arguments-read-and-assignment.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:30: assertEquals(42, function(){
return arguments;}(42)[0],
space after arguments;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:32: assertEquals(42, function(){
return arguments;}(42)[0],
space after arguments;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:36: assertEquals(42, function(){
if(arguments) return 42;}(),
space after 42;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:38: assertEquals(42, function(){
return arguments || true;}(42)[0],
space after true;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:40: assertEquals(true, function(){
return arguments && [true];}(42)[0],
space after [true];

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:42: assertEquals(42, function(){
arguments = 42; return 42;}(),
space after 42;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:74: 
In all of the above I would prefer having the function on its own line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:125: arguments[1] = 11;
new line before function, and move last argument to assertEquals down on
individual line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:154: }() + b;
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:159: function weirdargs(a,b,c) {
if (!a) return arguments;
new line before function body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:160: return [b[2],c]; }
move } down

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js
File test/mjsunit/array-concat.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js#ne...
test/mjsunit/array-concat.js:207: arr2.push("X");
indention (I don't think 4 indent + 2 for the function makes a lot of sense here
:-) )

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js#ne...
test/mjsunit/array-concat.js:216: arr2[500000] = "X";
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js#ne...
test/mjsunit/array-concat.js:228: function mkGetter(i) { return function() {
trace.push(i); }; }
remove last semicolon

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-elements-fro...
File test/mjsunit/array-elements-from-array-prototype-chain.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-elements-fro...
test/mjsunit/array-elements-from-array-prototype-chain.js:72: // Side-effects:
Array.prototype[3] now percolates into a[5] and Array.prototype[7]
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js
File test/mjsunit/array-iteration.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js...
test/mjsunit/array-iteration.js:53: assertArrayEquals([42,42],
a.filter(function(n, index, array) { array[index] = 43; return 42 == n; }));
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js...
test/mjsunit/array-iteration.js:59: assertArrayEquals([], a.filter(function(n,
index, array) { array.push(n+1); return n == 2; }));
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js...
test/mjsunit/array-iteration.js:182: assertArrayEquals(result, a.map(function(n,
index, array) { array.push(n); return n + 1;}));
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js...
test/mjsunit/array-iteration.js:212: assertTrue(a.some(function(n, index, array)
{ array[index] = n + 1; return n == 2; }));
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-iteration.js...
test/mjsunit/array-iteration.js:217: assertFalse(a.some(function(n, index,
array) { array.push(42); return n == 42; }));
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/null-comp...
File test/mjsunit/compiler/null-compare.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/null-comp...
test/mjsunit/compiler/null-compare.js:29: if (x == null) { return true;
this did not get much better :-)

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/null-comp...
test/mjsunit/compiler/null-compare.js:39: if (x === null) { return true;
neither did this

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-2.js
File test/mjsunit/compiler/regress-2.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-2...
test/mjsunit/compiler/regress-2.js:31: {
move this up as well

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-5.js
File test/mjsunit/compiler/regress-5.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-5...
test/mjsunit/compiler/regress-5.js:35: if (y == 0) { break bar;
put on new line and use {} in else as well

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-8.js
File test/mjsunit/compiler/regress-8.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-8...
test/mjsunit/compiler/regress-8.js:27: 
This file needs more love:
space after ,
space around +
space around ==
space around =
space round >=
jspace after if

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-8...
test/mjsunit/compiler/regress-8.js:44: function O() { this.append =
function(a,b,c,d,e) { return a + b + c + d + e; }; }
long line (and the looks better on individual line no matter what)

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-8...
test/mjsunit/compiler/regress-8.js:85:
LA+(a.Un+(zE+(Fp+(LA+(a.Im+(zE+(Mob+(LA+(a.total+zE))))))))),
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-s...
File test/mjsunit/compiler/regress-serialized-slots.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/compiler/regress-s...
test/mjsunit/compiler/regress-serialized-slots.js:38: {
move { up (or function down), also indention below seems wrong with this here, I
would move the function down with 4 indent and then the normal 2 indent for
function body, alternatively, just move } up and then have 2 indent

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace-te...
File test/mjsunit/debug-backtrace-text.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace-te...
test/mjsunit/debug-backtrace-text.js:76: if (event == Debug.DebugEvent.Break) {
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace-te...
test/mjsunit/debug-backtrace-text.js:83:
assertEquals("#<Point>.distanceTo(p=#<Point>)",
exec_state.frame(0).invocationText());
long line + two next lines

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace-te...
test/mjsunit/debug-backtrace-text.js:94: assertEquals("createPoint(x=0, y=0)",
exec_state.frame(1).invocationText());
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js
File test/mjsunit/debug-backtrace.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:126: json =
'{"seq":0,"type":"request","command":"backtrace","arguments":{"fromFrame":1,"toFrame":3}}';
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:144: json =
'{"seq":0,"type":"request","command":"backtrace","arguments":{"fromFrame":0,"toFrame":2,
"bottom":true}}';
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:176: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":0}}';
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:185: assertEquals('number',
response.lookup(frame.arguments[0].value.ref).type);
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:190: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":1}}';
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:201: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":3}}';
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints.js
File test/mjsunit/debug-breakpoints.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints....
test/mjsunit/debug-breakpoints.js:32: function f() {a=1;b=2;}
move body to (two) individual lines and space around =
if this is due to breakpoint positions just leave as is

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints....
test/mjsunit/debug-breakpoints.js:37: 
there seems to be missing a lot of spaces below, if this is to ease the tests
depending on position I am fine with leaving this as is

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-changebreakp...
File test/mjsunit/debug-changebreakpoint.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-changebreakp...
test/mjsunit/debug-changebreakpoint.js:72: testArguments(dcp, '{"breakpoint":' +
(breakpoint + 1) + ',"condition":"false"}', false);
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-version.js
File test/mjsunit/debug-version.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-version.js#n...
test/mjsunit/debug-version.js:64:
assertTrue(!!(version_string.match(version_pattern)), "unexpected format of
version: " + version_string);
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js
File test/mjsunit/div-mod.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode46
test/mjsunit/div-mod.js:46: var div_func = this.eval("(function(left) { return
left / " + divisor + "; })");
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode47
test/mjsunit/div-mod.js:47: var mod_func = this.eval("(function(left) { return
left % " + divisor + "; })");
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode100
test/mjsunit/div-mod.js:100: if (dividend < 0) { dividend = -dividend; sign =
-1; }
body on individual lines

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode207
test/mjsunit/div-mod.js:207: mod_func = this.eval("(function(left) { return left
% " + divisors[j]+ "; })");
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode208
test/mjsunit/div-mod.js:208: assertEquals((mod_func)(left_operands[i]),
left_operands[i] % divisors[j]);
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode209
test/mjsunit/div-mod.js:209: assertEquals((mod_func)(-left_operands[i]),
-left_operands[i] % divisors[j]);
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode215
test/mjsunit/div-mod.js:215: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/div-mod.js#newcode217
test/mjsunit/div-mod.js:217: [0, 0, 0, 8, 24, 56, 120, 120, 120, 632, 1656,
1656, 5752, 5752, 22136, 22136, 22136, 22136, 284280, 284280, 1332856, 3430008,
3430008, 3430008, 3430008, 36984440, 36984440, 36984440, 305419896, 305419896,
305419896],
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/for-in.js
File test/mjsunit/for-in.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/for-in.js#newcode61
test/mjsunit/for-in.js:61: a[Math.pow(2,30)-1] = 0;
space after , and around -

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/for-in.js#newcode63
test/mjsunit/for-in.js:63: a[Math.pow(2,31)-1] = 0;
space after , and around -

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/for-in.js#newcode90
test/mjsunit/for-in.js:90: a = [1,2,3,4];
space after ,

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/html-comments.js
File test/mjsunit/html-comments.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/html-comments.js#n...
test/mjsunit/html-comments.js:47: var x = 1; x <! x--;
why not ; here and below

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/json.js
File test/mjsunit/json.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/json.js#newcode30
test/mjsunit/json.js:30: assertEquals("1979-01-11T08:00:00.000Z", new
Date("1979-01-11 08:00 GMT").toJSON());
long line + below

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/json.js#newcode339
test/mjsunit/json.js:339: }
above: move body down to individual lines + move else if up on same line as }

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/json.js#newcode340
test/mjsunit/json.js:340: else if (string == '\r') expected = '\\r';
also use {} for last else if and move up on previous line after {

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/leakcheck.js
File test/mjsunit/leakcheck.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/leakcheck.js#newco...
test/mjsunit/leakcheck.js:34: if (n > 0) { return fac(n - 1) * n;
body down, else up and {} in else

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/no-semicolon.js
File test/mjsunit/no-semicolon.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/no-semicolon.js#ne...
test/mjsunit/no-semicolon.js:30: 
I don't think we wan't to add these in this file, see comment above

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/number-string-inde...
File test/mjsunit/number-string-index-call.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/number-string-inde...
test/mjsunit/number-string-index-call.js:28: var callbacks = [ function()
{return 'foo';}, "nonobject", /abc/ ];
space after { and before }

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/object-create.js
File test/mjsunit/object-create.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/object-create.js#n...
test/mjsunit/object-create.js:66: function valueGet() { ctr5++; return 3; }
individual lines for body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/object-create.js#n...
test/mjsunit/object-create.js:67: function getterGet() { ctr5++; return
function() { return ctr6++; }; }
individual lines for body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/object-define-prop...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/object-define-prop...
test/mjsunit/object-define-property.js:395: function get(){return this.x;}
spaces around the body + below (I wonder who wrote this :-) )

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-10...
File test/mjsunit/regress/regress-1099.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-10...
test/mjsunit/regress/regress-1099.js:30: // Test that LApplyArguments lithium
instruction restores context after the call.
long line

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-11...
File test/mjsunit/regress/regress-1170187.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-11...
test/mjsunit/regress/regress-1170187.js:76: (function(x,y){var a,b,c; debugger;
return 3;})();
spaces after ,

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-12...
File test/mjsunit/regress/regress-1207276.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-12...
test/mjsunit/regress/regress-1207276.js:33: function X(){String(Glo0al);}
space around body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-12...
File test/mjsunit/regress/regress-1257.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-12...
test/mjsunit/regress/regress-1257.js:37: case -1:
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-29...
File test/mjsunit/regress/regress-294.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-29...
test/mjsunit/regress/regress-294.js:35: if (x == "kat") { x = "kat";
body down else up

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-47...
File test/mjsunit/regress/regress-475.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-47...
test/mjsunit/regress/regress-475.js:28: assertEquals(1, (function (){return
1|-1%1;})());
spaces around body of function and around operators

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-52...
File test/mjsunit/regress/regress-524.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-52...
test/mjsunit/regress/regress-524.js:32: for (var j = 0; j < i; j++) { var o =
{}; o.x = 42; delete o.x; a[j] = o; }
put body on individual lines

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-78...
File test/mjsunit/regress/regress-780423.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-78...
test/mjsunit/regress/regress-780423.js:31: };
move }; up

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-79...
File test/mjsunit/regress/regress-799761.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-79...
test/mjsunit/regress/regress-799761.js:55: }
else up and add {} for else

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-81...
File test/mjsunit/regress/regress-815.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-81...
test/mjsunit/regress/regress-815.js:46: o[+o](1,2,3);
spaces after ,

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
File test/mjsunit/regress/regress-85177.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:30: gW=gH=175;
var?

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:31: g=[];
var?

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:34: var l=[];
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:42: if(a<0||b<0||a>=gW||b>=gH) {
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:49: for(var a=[],f=0; f<gW; f++){
indention

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:53: for(var i=-1; i<=1; i++)
add {} around for body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-85...
test/mjsunit/regress/regress-85177.js:54: for(var j=-1; j<=1; j++)
add {} around for body

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-89...
File test/mjsunit/regress/regress-892742.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-89...
test/mjsunit/regress/regress-892742.js:35: return;/* Counts as line-terminator
whitespace.
according to comment we should not add this here

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-89...
test/mjsunit/regress/regress-892742.js:40: return;// Comment doesn't include
line-terminator at end.
don't add here

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-93...
File test/mjsunit/regress/regress-931.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/regress/regress-93...
test/mjsunit/regress/regress-931.js:33: 2: function (x, y) { return x - y;} };
space after ;

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/simple-constructor.js
File test/mjsunit/simple-constructor.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/simple-constructor...
test/mjsunit/simple-constructor.js:126: this.x=1;
space around =

[Kevin Millikin (Chromium), 2011-12-08 11:20:51]

Drive by: is this necessary?

For failing tests, it's sometimes nice to ask the person who last modified the
code what the code is intended to test.

This makes that person you for a number of tests.

http://codereview.chromium.org/8888006/diff/1/src/array.js
File src/array.js (right):

http://codereview.chromium.org/8888006/diff/1/src/array.js#newcode1
src/array.js:1: // Copyright 2010 the V8 project authors. All rights reserved.
Here and elsewhere 2011.

[danno, 2011-12-08 11:53:16]

I have to agree with Kevin. These are purely cosmetic changes (full disclosure:
I only randomly sampled, I don't have the time to go through all). If such
changes were on the C/C++ code base to ensure maintainability, that would be one
thing, but the tests rarely get changed, and so I don't see the net added value
of this enormous CL. Please tell me that I am missing something?
On 2011/12/08 11:20:51, Kevin Millikin wrote:
> Drive by: is this necessary?
> 
> For failing tests, it's sometimes nice to ask the person who last modified the
> code what the code is intended to test.
> 
> This makes that person you for a number of tests.
> 
> http://codereview.chromium.org/8888006/diff/1/src/array.js
> File src/array.js (right):
> 
> http://codereview.chromium.org/8888006/diff/1/src/array.js#newcode1
> src/array.js:1: // Copyright 2010 the V8 project authors. All rights reserved.
> Here and elsewhere 2011.

[Lasse Reichstein, 2011-12-08 12:33:18]

Mostly only responded to comments I didn't agree with entirely.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
File test/mjsunit/arguments-read-and-assignment.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/arguments-read-and...
test/mjsunit/arguments-read-and-assignment.js:74: 
Feel free. I'll try not to expand the scope of this CL too much.
Also, it's not just functions, but function applications. It's hard enough to
read as is.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js
File test/mjsunit/array-concat.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/array-concat.js#ne...
test/mjsunit/array-concat.js:228: function mkGetter(i) { return function() {
trace.push(i); }; }
No, it ends the return statement.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js
File test/mjsunit/debug-backtrace.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:126: json =
'{"seq":0,"type":"request","command":"backtrace","arguments":{"fromFrame":1,"toFrame":3}}';
Single string JSON literal. I don't want to start splitting it (and change it
from a sequential string to a cons string).

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:144: json =
'{"seq":0,"type":"request","command":"backtrace","arguments":{"fromFrame":0,"toFrame":2,
"bottom":true}}';
ditto.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:176: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":0}}';
ditto.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:190: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":1}}';
ditto

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-backtrace.js...
test/mjsunit/debug-backtrace.js:201: json =
'{"seq":0,"type":"request","command":"frame","arguments":{"number":3}}';
ditto

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints.js
File test/mjsunit/debug-breakpoints.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints....
test/mjsunit/debug-breakpoints.js:32: function f() {a=1;b=2;}
Actual function body is being used in tests below. Better leave it be.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/debug-breakpoints....
test/mjsunit/debug-breakpoints.js:37: 
Again the missing space is because it refers to the actual body above.
I'm not about to rewrite the the checks of the test in this CL.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/html-comments.js
File test/mjsunit/html-comments.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/html-comments.js#n...
test/mjsunit/html-comments.js:47: var x = 1; x <! x--;
Actually, it shouldn't be above either. This test checks that <!-- begins a
comment, and isn't an operator, so it relies on ASI.

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/no-semicolon.js
File test/mjsunit/no-semicolon.js (right):

http://codereview.chromium.org/8888006/diff/1/test/mjsunit/no-semicolon.js#ne...
test/mjsunit/no-semicolon.js:30: 
Whoops, agree.

[Lasse Reichstein, 2011-12-08 12:41:24]

You have a point. The changes should be entirely cosmetic, and is merely to
bring our existing code closer to the coding standard that new code should use
(and the old code just shows that we haven't been that good at sticking to so
far). 
It should also have the advantage of being a good example for new code - if new
authors look at old code to see how to write it, the old code should be a good
example.

The tests might not be worth it though.

Maybe I should just restrict the fixups to the internal javascript files
(src/*.js)? These are being actively developed, where the tests are write-once
and only look back if they fail.