I'm curious, has there been any measurement/data as to the effect this has on output size?
On Mon, Jan 17, 2011 at 1:18 PM, <[email protected]> wrote: > 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 > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
