This patch took me hours to absorb. It sounds simple to modify the compiler to emulate stacks, but understanding precisely where the stack updates need to happen is difficult!
The implementation is very clean. There are just two nits in the comments. The line number recording looks like it should have a couple of exceptions where it includes a line-number updating statement just to be safe. Most significantly, it looks to me like one thing is missing from the finally block handling, and a great deal of the try-finally handling in there could safely be omitted. Please check out the comments on this and see if you agree. http://gwt-code-reviews.appspot.com/47816/diff/1/5 File dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java (right): http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode132 Line 132: // $stack[$stackDepth = stackIndex] = currentFunction This is safe, but why is it necessary? I would have thought that $stackDepth = stackIndex is enough. Shouldn't $stack[stackIndex] be the same until this function decrements stackIndex and returns? http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode174 Line 174: * exitingEarly && $stackDepeth = stackIndex - 1; There is no way to know for sure! It could depend on whether the try block throws an exception or simply fell off its end. Further, the throw might not be apparent in the code; it could have been thrown by some other function that the try block calls. Maybe this pop code is not needed. See my next comment. Is it even necessary to bother? See my next comment. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode184 Line 184: * the stack depth. This is a clever approach, and would seem to mean that it's not necessary at the *throw* site to update the emulated stack. Instead, it gets updated as soon as user code (as opposed to JSVM code or browser API code) starts running again. The only places user code can restart after an exception are catch blocks and finally blocks. Here you describe updating catch blocks. Wouldn't finally blocks need the same treatment? Going on from there, since catch and finally blocks reset the stack at their entry, is it necessary any longer to bother popping in finally blocks? That looks like a vestige from an earlier approach where exception throws did cause immediate stack updates. The newer approach appears to be that pushes happen at function entry, pops happen at function exit, and just-in-case stack resets happen at the top of finally and catch blocks. As an added bonus, the code should be simpler, because exitBlock and earlyExitName variables are no longer needed. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode204 Line 204: * arbitrary point. It's not an "arbitrary" point. If I understand correctly, it is if an exception occurs at the currently visited location? This variable then gets updated by visit() methods so that it stays accurate. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode360 Line 360: earlyExitNames.remove(exitBlock); This statement looks strange. Is it supposed to happen before the previous one? http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode400 Line 400: // Don't need a pop after a throw or break statement There are other cases where a function might not fall off the end. For example, there might be an if statement, and both branches might end with a return or a throw. I think an extra pop would be fine, because JavaScript is looser than Java about unreachable code. However, it is worth at least commenting that the point here is not to catch all cases, but to remove the excess pops in the most common cases. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode604 Line 604: private String baseName(String x) { The convention seems to be that "x" is an AST node. This variable holds a "fileName" or the like. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode620 Line 620: // Same location; ignore This can give wrong line numbers for looping statements, because the visitor order is a subset of the possible control-flow paths. Here's an example: for (int i = 0; i < someFunction(); i++) { doStuff(i); } Suppose someFunction() throws an exception on its *second* invocation but not its first: The line numbers are then incorrect. To correct this, a simple way would be to add paranoid extra line-number updaters for for loops and while statements. http://gwt-code-reviews.appspot.com/47816/diff/1/5#newcode663 Line 663: private boolean recordLineNumbers = Boolean.getBoolean("gwt.dev.stack.recordLineNumbers"); There is now a module CompilerParameters.gwt.xml that has misc. compiler settings that most users won't touch. While it's not a big deal, it seems helpful to keep things to the smallest scope that makes sense. Module scope is smaller than property scope. http://gwt-code-reviews.appspot.com/47816 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
