No patch attached, just comment replies.
http://gwt-code-reviews.appspot.com/169801/diff/1/2 File dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java (right): http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode43 Line 43: private class DuplicateFunctionBodyRecorder extends JsVisitor { On 2010/03/10 20:54:09, scottb wrote:
Fields should all be "final".
Done. http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode45 Line 45: Stack<JsExpression> contextStack = new Stack<JsExpression>(); On 2010/03/10 20:54:09, scottb wrote:
Suggest "Stack<JsNameRef> invocationQualifiers" and guard pushes w/ invocation.getQualifier instanceof JsNameRef?
Could do this, but I'll have to guard pop() as well. http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode123 Line 123: private Set<JsName> blacklist; On 2010/03/10 20:54:09, scottb wrote:
These should both be "final".
Done. http://gwt-code-reviews.appspot.com/169801/diff/1/3 File dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java (right): http://gwt-code-reviews.appspot.com/169801/diff/1/3#newcode21 Line 21: public class JsDuplicateFunctionRemoverTest extends OptimizerTestBase { I agree that it's better, I've made the mistake of putting assertEquals() arguments in the wrong order sometimes among other things, other issues include the need to think about where ToString() of the JsAST puts newlines and where it doesn't (ugh). I think maybe we should put that change in a followup patch for OptimizerTestBase complete and retro-patch the other test cases. On 2010/03/10 20:54:09, scottb wrote:
LGTM, but I have a non-blocking style suggestion... I think the test
ends up
more easily readable using the optimize(...).into(...) pattern in DeadCodeEliminationTest. What do you think?
http://gwt-code-reviews.appspot.com/169801 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
