Code cleanup: break or shorten long lines, misc cleanup. (issue 7686047)

[usrb...@yahoo.com]

Reviewers: arv-chromium, slightlylate,

Message:
I try not to dilute my patches with formatting changes, so I finally
decided to do one patch to concentrate all the dilution in one place. So
to speak.

The computer is a harsh taskmaster. I set up my .git/hooks/pre-commit so
that I could be immediately notified about any long lines, but sometimes
you want to ignore them and just do the patch. I may go ahead and change
it so that by default it only checks the lines I've added.

Turns out the V8 codebase (C++ part) is perfectly clean in this
respect. Looking a little closer, it seems they use a lint tool. That
idea may be something worth revisiting eventually.

I really want to try to minimize time spent doing (and time others spend
reviewing) formatting-only patches, so I may end up adopting that as a
compromise between code purity and coding efficiency.

----

Anyway. I went through the results of

# Note: uses a bash-only feature.
awk '
length($0) > 80 && !/https?:/ {
if(f!=FILENAME){
f=FILENAME;
print "\n"f;
}
print " "$0;
}' $(git ls-files !(bin|third_party)) | less

and picked the low-hanging fruit.

The messy changes are in patch set 1, and the messiest of those are in
Makefile. Patch set 2 only breaks a few module import/export lines.

#--cut--
dl_check() {
local f=$(basename $1)
test -e $f || curl -sO $1
openssl sha1 < $f | grep -q "= $2" && return 0
echo sha1 mismatch!
return 1
}

git checkout -b issue7686047-long-lines-partial-fix

dl_check https://codereview.appspot.com/download/issue7686047_4001.diff
\
cf3df2dd2965edf9 && git apply issue7686047_4001.diff && make test

# Only two changes in the output.
#
# src/codegeneration/generator/State.js
# Removing an extra createStatementList import
# src/syntax/ParseTreeValidator.js
# A '+' joining a string that was split across two lines.
git diff bin/traceur.js
#--cut--



https://codereview.appspot.com/7686047/diff/1/Makefile
File Makefile (left):

https://codereview.appspot.com/7686047/diff/1/Makefile#oldcode4
Makefile:4: GENSRC = \
I decided to just go ahead and sort this to keep things consistent as
this list slowly (maybe?) grows.

https://codereview.appspot.com/7686047/diff/1/Makefile#oldcode58
Makefile:58: src/syntax/trees/ParseTrees.js: src/syntax/trees/trees.json
build/build-parse-trees.js
By renaming all files according to the pattern
build-parse-trees.js => src/syntax/ParseTrees.js-builder.js
you could replace all these rules with
%.js: %.js-builder.js src/syntax/trees.js
node $^ > $@

https://codereview.appspot.com/7686047/diff/1/Makefile#oldcode74
Makefile:74: uglifyjs bin/traceur.js --compress
dead_code=true,unused=true,sequences=true,join_vars=true,evaluate=true,booleans=true,conditionals=true
-m -o $@
According to https://github.com/mishoo/UglifyJS2 these are all default
options. Alternatively, you can get the effect of an unbroken command
line arg by:

bin/traceur.ugly.js: bin/traceur.js
# The important thing is no indentation; just the leading tab.
uglifyjs bin/traceur.js --compress dead_code=true,unused=true,\
sequences=true,join_vars=true,evaluate=true,booleans=true,\
conditionals=true -m -o $@

https://codereview.appspot.com/7686047/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7686047/diff/1/Makefile#newcode42
Makefile:42: cd test/bench/esprima; git reset --hard 1ddd7e0524d09475
The birthday paradox doesn't apply here, so even 64 bits is overkill,
really.

https://codereview.appspot.com/7686047/diff/1/src/codegeneration/generator/State.js
File src/codegeneration/generator/State.js (left):

https://codereview.appspot.com/7686047/diff/1/src/codegeneration/generator/State.js#oldcode23
src/codegeneration/generator/State.js:23: createStatementList,
I got curious at how this got in here in the first place, and it looks
like it was here from the beginning (commit 77a9a5f500946a09) of this
file's git history. Who knows what the story was prior to that.

https://codereview.appspot.com/7686047/diff/1/src/node/inline-module.js
File src/node/inline-module.js (right):

https://codereview.appspot.com/7686047/diff/1/src/node/inline-module.js#newcode122
src/node/inline-module.js:122: path.relative(path.join(__dirname,
'../..'), codeUnit.url));
The change that prompted it all. My pre-commit hook kept buzzing me on
the line length, so I finally just shortened it.

Description:
Code cleanup: break or shorten long lines, misc cleanup.

BUG=None
TEST=None


Please review this at https://codereview.appspot.com/7686047/

Affected files:
M Makefile
M src/codegeneration/BlockBindingTransformer.js
M src/codegeneration/CoverFormalsTransformer.js
M src/codegeneration/ParseTreeFactory.js
M src/codegeneration/ProgramTransformer.js
M src/codegeneration/generator/BreakContinueTransformer.js
M src/codegeneration/generator/BreakState.js
M src/codegeneration/generator/FinallyFallThroughState.js
M src/codegeneration/generator/State.js
M src/node/inline-module.js
M src/options.js
M src/runtime/modules.js
M src/semantics/ModuleAnalyzer.js
M src/semantics/symbols/Project.js
M src/semantics/symbols/Symbol.js
M src/syntax/ParseTreeValidator.js
M src/syntax/Scanner.js
M src/traceur.js

[a...@chromium.org]

Thanks


https://codereview.appspot.com/7686047/diff/4001/Makefile
File Makefile (right):

https://codereview.appspot.com/7686047/diff/4001/Makefile#newcode77
Makefile:77: uglifyjs bin/traceur.js --compress -m -o $@
Why were these options changed?

https://codereview.appspot.com/7686047/diff/4001/src/node/inline-module.js
File src/node/inline-module.js (right):

https://codereview.appspot.com/7686047/diff/4001/src/node/inline-module.js#newcode122

src/node/inline-module.js:122: path.relative(path.join(__dirname,
'../..'), codeUnit.url));

does this do the right thing on Windows?

https://codereview.appspot.com/7686047/

[usrb...@yahoo.com]

Does the possible change suggested in

https://codereview.appspot.com/7686047/diff/1/Makefile#oldcode58

seem reasonable? (Copy-paste for convenience)

By renaming all files according to the pattern
build-parse-trees.js => src/syntax/ParseTrees.js-builder.js
you could replace all these rules with
%.js: %.js-builder.js src/syntax/trees.js
node $^ > $@

https://codereview.appspot.com/7686047/diff/4001/Makefile
File Makefile (right):

https://codereview.appspot.com/7686047/diff/4001/Makefile#newcode77
Makefile:77: uglifyjs bin/traceur.js --compress -m -o $@

On 2013/03/21 21:09:22, arv-chromium wrote:
> Why were these options changed?

Patch Set 1 comment copy-paste for convenience:

According to https://github.com/mishoo/UglifyJS2 these are all default
options. Alternatively, you can get the effect of an unbroken command
line arg by:

bin/traceur.ugly.js: bin/traceur.js
# The important thing is no indentation; just the leading tab.
uglifyjs bin/traceur.js --compress dead_code=true,unused=true,\
sequences=true,join_vars=true,evaluate=true,booleans=true,\
conditionals=true -m -o $@

----

The line continuation alternative is reasonable if you want to specify
options explicitly, or if there is a version of uglifyjs in the wild
that doesn't use these options by default.

https://codereview.appspot.com/7686047/diff/4001/src/node/inline-module.js
File src/node/inline-module.js (right):

https://codereview.appspot.com/7686047/diff/4001/src/node/inline-module.js#newcode122
src/node/inline-module.js:122: path.relative(path.join(__dirname,
'../..'), codeUnit.url));

On 2013/03/21 21:09:22, arv-chromium wrote:
> does this do the right thing on Windows?

That's a good question. The glib (but non-gnu) answer would be that if
it doesn't, it really should, but seriously:

// path.join ends by running path.normalize over the result.
https://github.com/joyent/node/blob/master/lib/path.js#L234
// This is the key line in path.normalize:
// normalizeArray is separator-agnostic, so everything gets converted
to
// backslash on windows.
https://github.com/joyent/node/blob/master/lib/path.js#L185
tail = normalizeArray(tail.split(/[\\\/]+/).filter(function(p) {
return !!p;
}), !isAbsolute).join('\\');

Okay, I wouldn't feel right leaving it just at theory, so I went ahead
and installed the windows version of node:

0958d68900b80419e2af7a5448c4a451af06d0b4 x64/node-v0.10.1-x64.msi

> path.join('hello', '../..', 'world')
'..\\world'

https://codereview.appspot.com/7686047/

[a...@chromium.org]

LGTM

Committed as a51e03ddfbcc2caa114d932e40b43aa22fda960e

https://codereview.appspot.com/7686047/