Mostly LGTM, except for the concern about JNewInstance.isEmpty(). It looks like JNewInstance.isEmpty() should be computed within RemoveEmptySuperCalls to be efficient, or for that matter, to reliably terminate.
There are also some nits. http://gwt-code-reviews.appspot.com/159803/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/ast/JConstructor.java (right): http://gwt-code-reviews.appspot.com/159803/diff/1/3#newcode31 Line 31: private boolean isEmpty = false; Nice, that will be handy. http://gwt-code-reviews.appspot.com/159803/diff/1/3#newcode38 Line 38: public JConstructor(SourceInfo info, JClassType enclosingType, You probably meant this to be default access. http://gwt-code-reviews.appspot.com/159803/diff/1/3#newcode64 Line 64: public boolean isEmpty() { It's worth a comment that this method is currently expensive. It's also worth a TODO to make it less expensive by only recomputing this at places it could have possibly changed. http://gwt-code-reviews.appspot.com/159803/diff/1/3#newcode80 Line 80: if (expr instanceof JMethodCall) { Check for JNewInstance instead of JMethodCall? Then testing instanceof JConstructor shouldn't be needed. http://gwt-code-reviews.appspot.com/159803/diff/1/3#newcode84 Line 84: return isEmpty = ((JConstructor) target).isEmpty(); Can't this loop, if two constructors call each other? Maybe we need to implement the TODO already, and move the computation to be done, say, after pruning. It's easy to deal efficiently with cycles in the call graph if we do (yet another :( ) global walk over all constructor bodies. It could also be rolled into your follow-up patch dealing with empty constructors, if it starts looking like too much work. http://gwt-code-reviews.appspot.com/159803/diff/1/4 File dev/core/src/com/google/gwt/dev/jjs/ast/JGwtCreate.java (right): http://gwt-code-reviews.appspot.com/159803/diff/1/4#newcode38 Line 38: if (ctor instanceof JConstructor) { That's ever so much nicer a way to check for constructor calls. http://gwt-code-reviews.appspot.com/159803/diff/1/7 File dev/core/src/com/google/gwt/dev/jjs/ast/JNewInstance.java (right): http://gwt-code-reviews.appspot.com/159803/diff/1/7#newcode65 Line 65: public boolean hasClinit() { Looks great. http://gwt-code-reviews.appspot.com/159803/diff/1/8 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/159803/diff/1/8#newcode993 Line 993: true, false); Makes sense. Perhaps we should make this a real method that does something and returns null. It seems like each non-trivial patch needs a new special case for the null method and/or the null field. http://gwt-code-reviews.appspot.com/159803/diff/1/25 File dev/core/src/com/google/gwt/dev/jjs/impl/RemoveEmptySuperCalls.java (right): http://gwt-code-reviews.appspot.com/159803/diff/1/25#newcode50 Line 50: ctx.replaceMe(multi.makeStatement()); It's not something that needs to happen now, but this three-way branch would work really well as a method in Simplifier. The type signature would be Simplifier.multi(List<JExpression> exprs); . We keep rewriting this logic because we don't want to have to wait until DeadCodeElimination swings around to do the optimization. http://gwt-code-reviews.appspot.com/159803 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
