LGTM, just nits.  No need to re-review.

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002
File dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java
(right):

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6002#newcode143
dev/core/src/com/google/gwt/dev/js/JsDuplicateCaseFolder.java:143:
sb.append("\0"); // separate statements with an otherwise illegal char
I would actually use '\n' here so that the string is easily readable in
a debugger.

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003
File dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java (right):

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6003#newcode50
dev/core/src/com/google/gwt/dev/js/ast/JsBlock.java:50: public boolean
unconditionalControlBreak() {
This work correctly because of the implementation of
JBreakStatement.unconditionalControlBreak(), which never counts labelled
break statements as breaks.  Could you add a comment here or there (or
both?) about this dependency between the two?

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004
File dev/core/src/com/google/gwt/dev/js/ast/JsIf.java (right):

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6004#newcode86
dev/core/src/com/google/gwt/dev/js/ast/JsIf.java:86: }
This is handy, but DeadCodeElimination should have already taken care of
these cases.  I would argue for going with the computationally cheaper
(and simpler) implementation here if it doesn't actually improve the
output.

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005
File dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java
(right):

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode85
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:85:
private void check(String input, String expected) throws Exception {
Total nit, but I would reverse the argument order to match the order of
assertEquals().

http://gwt-code-reviews.appspot.com/1286801/diff/5002/6005#newcode87
dev/core/test/com/google/gwt/dev/js/JsDuplicateCaseFolderTest.java:87:
String optimized = optimize(expected);
You probably don't actually want to optimize this, right?  Suggest:

// Generate canonical expected output (no optimizers)
String expectedOutput = super.optimize(expected, new Class[0]);

http://gwt-code-reviews.appspot.com/1286801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to