Comment re: clusterer below. Working on the formatting now... On Mon, May 23, 2011 at 2:07 PM, <zun...@google.com> wrote:
> > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java > File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java > (right): > > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode1046 > dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1046: > infoMap = ((JsReportGenerationVisitor) v).getSourceInfoMap(); > I'm wondering if this would be cleaner if we were to move the > 'clusterer' into JsReportGenerationVisitorWithSizeBreakdown > > Then in the JsReportGenerationVisitor constructor you could stash away > infoMap and override clusterer with this extra reordering work > > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode1057 > dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1057: > JsFunctionClusterer clusterer = > shouldn't this be declared inside the if() block? Anyway, I'm proposing > to move it inside JsReportGenerationVisitorWithSizeBreakdown anyway. > The clusterer instance is used by the JsIEBlockTextTransformer constructor below that: dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1072: JsIEBlockTextTransformer ieXformer = new JsIEBlockTextTransformer(clusterer); So if it's declared inside the if() block, the scope won't work. Should the JsIEBlockTextTransformer stuff be moved inside JsReportGenerationVisitorWithSizeBreakdown, too, then? > > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode1088 > dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1088: > * This method shifts the Range indices that surround each JavaScript > expression after clustering > comments should be < 80 chars. If you setup your eclipse formatting as > in trunk/eclipse/README.txt, you should be able to use the eclipse > Source->Format tool to fix it. > > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode1097 > dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1097: > private static Map<Range, SourceInfo> > updateRangesAfterClustering(StatementRanges oldRanges, > Method out of order. See trunk/eclipse/README.txt If you have eclipse > setup, you can just use the Source->Sort Members... function to fix it. > > > http://gwt-code-reviews.appspot.com/1451801/diff/1/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode1107 > dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:1107: > statementShifts.put(new Range(originalStart, originalEnd), new > Range(permutedStart, permutedEnd)); > code links should be < 100 chars. again, autoformat will just fix this. > > > http://gwt-code-reviews.appspot.com/1451801/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
