My brain hurts now.
http://gwt-code-reviews.appspot.com/473801/diff/5001/6004 File dev/core/src/com/google/gwt/dev/shell/JsValueGlue.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6004#newcode63 dev/core/src/com/google/gwt/dev/shell/JsValueGlue.java:63: if (desiredType != null && !desiredType.isAssignableFrom(jsoType)) { Just move this code out to the single call site that needs it, in get(). http://gwt-code-reviews.appspot.com/473801/diff/5001/6005 File dev/core/src/com/google/gwt/dev/shell/rewrite/HostedModeClassRewriter.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6005#newcode46 dev/core/src/com/google/gwt/dev/shell/rewrite/HostedModeClassRewriter.java:46: * instantiable type.</li> This wants a major rewrite, naturally. http://gwt-code-reviews.appspot.com/473801/diff/5001/6005#newcode60 dev/core/src/com/google/gwt/dev/shell/rewrite/HostedModeClassRewriter.java:60: static class RewriterOracle { I like how you did this. It looks like, down the road, this could easily be redone to not use TypeOracle, but to use CCL or ASM+CompState. http://gwt-code-reviews.appspot.com/473801/diff/5001/6006 File dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode57 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:57: private class MyMethodAdapter extends AnalyzerAdapter { Somewhere in here, don't you need to rewrite things like new JsoSubtype[3] to new JavaScriptObject[3]? Also, what about visitVarInsn for storing into locals/params, and visitInsn for storing into arrays? http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode128 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:128: && rewriterOracle.isJsoSubtype(t.getInternalName())) { I think I see a problem here: IJsoIntf i = (JsoSubclass) jso; i.getClass() --> incorrectly returns JsoSubclass.class http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode136 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:136: && rewriterOracle.isJsoSubtype(t.getElementType().getInternalName())) { This goes away if we just prevent JsoSubclass[] from ever getting instantiated. http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode148 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:148: Method m = new Method(name, desc); Nasty, but wow, guess it works. Maybe a short overview up here? http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode272 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:272: * Casts to Object or JavaScriptObject should result in the canonical object Does a cast to JavaScriptObject require the canonical object? Or just casts to Object? I forget what we decided there. Seems like less code generally gets generated & executed if only Object has to be canonical. http://gwt-code-reviews.appspot.com/473801/diff/5001/6006#newcode356 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteJsoCasts.java:356: } else if (rewriterOracle.isJsoSubtype(internalName)) { BTW: the implementation of this method returns true for JSO itself, and for subtypes. This is the intent here? http://gwt-code-reviews.appspot.com/473801/diff/5001/6007 File dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteObjectComparisons.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6007#newcode39 dev/core/src/com/google/gwt/dev/shell/rewrite/RewriteObjectComparisons.java:39: public void visitJumpInsn(int opcode, Label label) { This is shocking enough to be worth a comment, that reference equality always requires a jump instruction. It's pretty surprising that code like this: boolean eq = (a == b); turns into something involve jumps. http://gwt-code-reviews.appspot.com/473801/diff/5001/6010 File dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode46 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:46: if (o == null) { o can never be null based on the single caller. http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode50 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:50: if (Object.class == jsoOrIntfType) { can't be Object either, because then desiredType.isAssignableFrom(jsoType) http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode54 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:54: if (jsoOrIntfType.isInstance(o)) { pretty sure this will never be true, either http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode63 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:63: return cast(o, jsoOrIntfType, targetJsoType); if (targetJsoType != null) return rewrap(..) http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode97 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:97: throw new ClassCastException( Can't you just return and let the caller throw the CCE? Actually, this method should boil down to approximately: if (targetJsoType == null || o == null || targetJsoType.isInstance(o) || !getJsoClass(o).isInstance(o)) { return o; } return rewrap(targetJsoType, o); I've arranged the tests in order of what I'd imagine is the likelihood and cost. http://gwt-code-reviews.appspot.com/473801/diff/5001/6010#newcode121 dev/core/src/com/google/gwt/dev/shell/rewrite/SingleJsoImplSupport.java:121: assert intf != null : "No interface type for instanceof check"; The most common case is when implJsoType == null. Suggest: return intf.isInstance(o) || ((implJsoType != null) && getJsoClass(o).isInstance(o)); No real need for the assert (there's a runtime check, even with -da) and you get an instant NPE anyway. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011 File dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode45 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:45: * <li>The zero-arg constructor is made public and makes the JavaScriptObject I guess this could also be turned into a 1-arg constructor that takes the hostedModeReference and assigns it, then we wouldn't have to do two reflective invocations to construct one. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode92 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:92: // Write the zero-arg constructor Code please? http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode105 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:105: mv.visitVarInsn(Opcodes.ALOAD, 0); DUP is your friend. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode118 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:118: desc = "(L" + JAVASCRIPTOBJECT_DESC + ";)V"; More code please. :) http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode244 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:244: // make the constructor public Shouldn't be necessary since it's only called from the rewrap method in the same class. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode267 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:267: mv.visitLocalVariable("this", "L" + getOriginalName() + ";", null, BTW: why do you have to do this for method params (as opposed to just true locals)? Is it for debugging? http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode282 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:282: * public static JsoSubclass rewrap$(JavaScriptObject jso) { What if jso is already the right type? Do we avoid the extra wrapper? http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode286 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:286: * JavaScriptObjet canonical = jso.canonical; Not positive, but I *think* the super ctor actually reads jso.canonical internally, so I don't think you even need to canonicalize it here. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode297 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:297: mv.visitVarInsn(Opcodes.ALOAD, 0); DUP here, then you don't need to push the null / do another aload below. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode375 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:375: private WriteJsoImpl(ClassVisitor cv) { You know, there's practically no point in sharing a common superclass at this point. More confusing than useful, really. http://gwt-code-reviews.appspot.com/473801/diff/5001/6011#newcode397 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteJsoImpl.java:397: protected String getOriginalName() { The method and field name is a misnomer now, since we don't actually rewrite the name any more. http://gwt-code-reviews.appspot.com/473801/diff/5001/6012 File dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java (right): http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode64 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:64: * o = JavaScriptObject.rewrap$((JavaScriptObject o); Need closing paren on cast op http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode115 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:115: toInternalName(SingleJsoImplSupport.class.getName()), "cast", Good candidate for a constant. http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode135 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:135: mv.visitCode(); I think this gets simpler if you use DUP... aload 0 // obj dup // obj, obj instanceof JSO // int, obj jump ifeq beforeReturn // obj checkcast JSO // JSO invoke JSO.rewarp // canonical JSO beforeReturn: // obj OR canonical JSO frame areturn http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode158 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:158: new Object[0]); All new Object[0] really want to use a shared constant. http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode242 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:242: mv.visitFrame(Opcodes.F_NEW, 0, new Object[0], 0, new Object[0]); Do you have to have a frame instruction at the start of a method? The doc makes it seem like you don't. http://gwt-code-reviews.appspot.com/473801/diff/5001/6012#newcode289 dev/core/src/com/google/gwt/dev/shell/rewrite/WriteSingleJsoSupportCode.java:289: * its interfaces. What does the generated method look like (in source)? http://gwt-code-reviews.appspot.com/473801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
