LGTM. Aside from documentation and naming, there is one question about which nodes are re-added to the worklist in the solver.
Can you post a large enough set of code to include an actual analysis and optimization? I don't think we should commit any of this code until there it is causing at least some improvement, however small. http://gwt-code-reviews.appspot.com/112811/diff/1/3 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/AnalysisSolver.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/3#newcode28 Line 28: public interface AnalysisSolver<N, E, T, G extends Graph<N, E, T, G>> { Why the interface? Unlike the other interfaces in this package, I don't see why a second implementation would ever coexist in the same code base. http://gwt-code-reviews.appspot.com/112811/diff/1/4 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/AnalysisSolverImpl.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode30 Line 30: * backwards working. Both of directions will always produce a fixed point, Since forward only influences the initial visit order of the nodes in the CFG, I don't think it's worth calling out in the class comment. Talk about it in the constructor, and say that what it does is affect the initial visit order. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode94 Line 94: Map<E, A> solution = iterate(newSubgraph, "subgraphSolution"? I find this method tricky, so careful names would help. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode112 Line 112: return result; No need to make a temp and return it. Just return it. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode127 Line 127: * Solve analysis Solve a non-integrated analysis. (Or, come up with a better name than non-integrated?) http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode142 Line 142: worklist.add(nodes.get(i)); I see a lot of random access on lists in this function. Making them ArrayLists instead of Lists would prevent accidental bad performance if anyone tries to substitute something else. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode167 Line 167: throw new IllegalArgumentException(); Be consistent with this exception message and the previous one? http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode181 Line 181: addSuccessors(g, worklist, node); Doesn't this add more nodes than necessary to the work list? If only one output edge changes, then only that node's target edge needs to be visited again. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode185 Line 185: for (int i = 0; i < inEdges.size(); i++) { Analogous questions here as for the previous loop. http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode205 Line 205: * Solve integrated analysis. Solve an integrated analysis by using an IntegratedFlowFunctionAdapter and recursing into {...@link #solve()} http://gwt-code-reviews.appspot.com/112811/diff/1/4#newcode327 Line 327: private <A extends Assumption<A>> Map<E, A> iterate(G g, This method deserves a method comment. http://gwt-code-reviews.appspot.com/112811/diff/1/5 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/Assumption.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/5#newcode30 Line 30: Self meet(Self value); The least upper bound is a *join*, not a meet. http://gwt-code-reviews.appspot.com/112811/diff/1/7 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/FlowFunction.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/7#newcode24 Line 24: * not clear yet. To clear up this paragraph, keep the first sentence, and then say the rest as: Typical flow functions update either outgoing assumptions (forward flow) or incoming assumptions (backward flow) but not both. http://gwt-code-reviews.appspot.com/112811/diff/1/10 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/IntegratedFlowFunction.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/10#newcode22 Line 22: * node transformation based on already computed assumptions. Do I undestand correctly that in typical cases, the replacement graph would be a single node, e.g. a nop node or a node that has fewer connecting edges? I started to say there are caching issue to consider here, but if the typical replacements are a single node then it doesn't matter. http://gwt-code-reviews.appspot.com/112811/diff/1/11 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/NodeAssumptions.java (right): http://gwt-code-reviews.appspot.com/112811/diff/1/11#newcode24 Line 24: * node, which allows us to use this class to represnt graph assumptions. represent. Although, given the second sentence, isn't this more accurately described as the assumptions about a *subgraph* ? The subgraph is often exactly one node, but it doesn't have to be. http://gwt-code-reviews.appspot.com/112811/diff/1/11#newcode26 Line 26: * @param <A> Please add a doc string. http://gwt-code-reviews.appspot.com/112811/diff/1/13 File dev/core/src/com/google/gwt/dev/jjs/impl/flow/package.html (right): http://gwt-code-reviews.appspot.com/112811/diff/1/13#newcode28 Line 28: with transformation as describied in "Composing Dataflow Analyses and described http://gwt-code-reviews.appspot.com/112811 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
