Bob, sorry for taking so long. I have to admit to myself I think I've been dragging my feet because I still feel very squirrelly about this whole thing. That being said, I reviewed the implementation as if I thought the goal were a good idea. :) I fervently hope that the deRPC win you're seeing (vs. deRPC but explicit code to read/assign fields) truly justifies this.
http://gwt-code-reviews.appspot.com/46801/diff/1041/24 File dev/core/src/com/google/gwt/dev/javac/ArtificialRescueChecker.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/24#newcode282 Line 282: return "Cannot refer to fields on " + "array or primitive types"; Why split? http://gwt-code-reviews.appspot.com/46801/diff/1041/24#newcode290 Line 290: return "Cannot refer to methods on " + "array or primitive types"; Ditto. http://gwt-code-reviews.appspot.com/46801/diff/1041/24#newcode313 Line 313: private boolean allowArtificialRescue = true; Slightly clearer to make this final and uninitialized, initialize in the appropriate ctor. http://gwt-code-reviews.appspot.com/46801/diff/1041/26 File dev/core/src/com/google/gwt/dev/jdt/AbstractCompiler.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/26#newcode183 Line 183: private void addAdditionalTypes(TreeLogger logger, String[] typeNames) { Thanks! http://gwt-code-reviews.appspot.com/46801/diff/1041/26#newcode504 Line 504: protected String[] doFindAdditionalTypesUsingArtificialRescues( @SuppressWarnings("unused") // overrider may use unused parameter http://gwt-code-reviews.appspot.com/46801/diff/1041/29 File dev/core/src/com/google/gwt/dev/jjs/ast/HasArtificialRescues.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/29#newcode25 Line 25: public interface HasArtificialRescues { Is there a reason for this type? You're not actually using it anywhere and there's only one implementor. http://gwt-code-reviews.appspot.com/46801/diff/1041/30 File dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/30#newcode35 Line 35: * The other nodes that this node should implicitly rescue. "Special serialization treatment." http://gwt-code-reviews.appspot.com/46801/diff/1041/31 File dev/core/src/com/google/gwt/dev/jjs/ast/JField.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/31#newcode82 Line 82: public void setDisposition(Disposition disposition) { Ugh. :( http://gwt-code-reviews.appspot.com/46801/diff/1041/32 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/32#newcode914 Line 914: if ("Z".equals(className) || "boolean".equals(className)) { What's this for? Primitives/arrays are illegal rescue targets anyway. http://gwt-code-reviews.appspot.com/46801/diff/1041/32#newcode1086 Line 1086: visitor.accept(new ArrayList<JArrayType>(allArrayTypes)); Same here, why do we need this? http://gwt-code-reviews.appspot.com/46801/diff/1041/34 File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaAST.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/34#newcode268 Line 268: private final CompilationUnitDeclaration[] goldenCuds; This is never read; so I think you can undo threading this through, fortunately. http://gwt-code-reviews.appspot.com/46801/diff/1041/34#newcode2512 Line 2512: field.setDisposition(JField.Disposition.VOLATILE); Is this absolutely necessary? This seems wrong-- the field is not really volatile, it's fixed upon instantiation, which is different from "might change at any time." In theory you could be killing a lot of optimizations. http://gwt-code-reviews.appspot.com/46801/diff/1041/34#newcode2520 Line 2520: outer : for (String methodName : methods) { Would JsniRefLookup help here? http://gwt-code-reviews.appspot.com/46801/diff/1041/36 File dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/36#newcode147 Line 147: boolean pruned; LG, but separate commit, right? http://gwt-code-reviews.appspot.com/46801/diff/1041/39 File dev/core/super/com/google/gwt/core/client/ArtificialRescue.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/39#newcode26 Line 26: * of the deRPC code and its use by third parties is not supported. There's also a core.client.impl package, would that be more appropriate? http://gwt-code-reviews.appspot.com/46801/diff/1041/39#newcode29 Line 29: @Retention(RetentionPolicy.SOURCE) Is this going to jive with IHM? I could foresee needed classfile retention. http://gwt-code-reviews.appspot.com/46801/diff/1041/39#newcode40 Line 40: String className(); Why is this a String? If we made this a Class and insisted on a class literal, it seems like it would save us a fair amount of work. For one thing, we wouldn't need to find additional types using magic, because JDT would pull it in through the class literal. Also, we'd never fail to lookup the target class, because JDT would have caught it at compile time. http://gwt-code-reviews.appspot.com/46801/diff/1041/40 File dev/core/test/com/google/gwt/dev/javac/ArtificialRescueCheckerTest.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/40#newcode25 Line 25: private StringBuilder targetClass; This won't actually do any caching because each test method is run within a new class instance. You'd have to make it static. Seems simpler not to bother caching. http://gwt-code-reviews.appspot.com/46801/diff/1041/41 File dev/core/test/com/google/gwt/dev/javac/CheckerTestCase.java (right): http://gwt-code-reviews.appspot.com/46801/diff/1041/41#newcode95 Line 95: } By the way... how the heck are you getting the compilation unit for @ArtificialRescue in there??? I debugged through and never saw where you're adding it in. http://gwt-code-reviews.appspot.com/46801 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
