[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
LGTM with comments 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#newcode504 Line 504: protected String[] doFindAdditionalTypesUsingArtificialRescues( Yeah, I'll update something in trunk/eclipse. 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: } After more thinking about it, I think binary-only-annotations support is allowing this to happen. http://gwt-code-reviews.appspot.com/46801/diff/5008/4010 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/46801/diff/5008/4010#newcode1087 Line 1087: visitor.accept(new ArrayList(allArrayTypes)); Could we just do a GenerateJavaScriptAst.endVisit(JProgram) to export these symbols manually? Otherwise you're needlessly slowing down the rest of the compile. http://gwt-code-reviews.appspot.com/46801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
Ignore patch set 5; the change to JProgram.traverse() is necessary. http://gwt-code-reviews.appspot.com/46801/diff/5008/4010 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/46801/diff/5008/4010#newcode1087 Line 1087: visitor.accept(new ArrayList(allArrayTypes)); I remembered why this is necessary once I rebased derpc against these changes. It allows GenerateJavaScriptAst.endVisit(JArrayType) to correctly export symbol data for array types. http://gwt-code-reviews.appspot.com/46801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
Updated.
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";
On 2009/07/03 04:23:38, scottb wrote:
> Why split?
I had used the "extract method" refactoring and promptly forgot about
reformatting the message.
http://gwt-code-reviews.appspot.com/46801/diff/1041/24#newcode313
Line 313: private boolean allowArtificialRescue = true;
Done.
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#newcode504
Line 504: protected String[]
doFindAdditionalTypesUsingArtificialRescues(
Done.
Can you publish your eclipse warnings/errors settings?
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 {
Only because when I started I wasn't sure if it the annotation would be
on just types, methods, or both.
Removed.
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.
Done.
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) {
No longer necessary based on your other comments; Changes to this file
have been reverted.
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)) {
Primitives are illegal rescues, but array types can be rescued;
B extends A;
If an RPC type uses A[], then we need to make sure that the compiled
module also includes support for B[].
http://gwt-code-reviews.appspot.com/46801/diff/1041/32#newcode1086
Line 1086: visitor.accept(new ArrayList(allArrayTypes));
This is no longer necessary.
Reverting all changes to JProgram.
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#newcode2512
Line 2512: field.setDisposition(JField.Disposition.VOLATILE);
It's no longer necessary given the current implementation of
CFA.rescue(JReferenceType) which handles fields.
http://gwt-code-reviews.appspot.com/46801/diff/1041/34#newcode2520
Line 2520: outer : for (String methodName : methods) {
Both the methods and fields loops have been collapsed into a single
loop.
http://gwt-code-reviews.appspot.com/46801/diff/1041/34#newcode2980
Line 2980: * @param goldenCuds TODO
Unthreaded.
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;
On 2009/07/03 04:23:38, scottb wrote:
> LG, but separate commit, right?
Yes; existing logic was hard to keep straight.
http://gwt-code-reviews.appspot.com/46801/diff/1041/38
File dev/core/src/com/google/gwt/dev/js/ast/JsFunction.java (right):
http://gwt-code-reviews.appspot.com/46801/diff/1041/38#newcode128
Line 128:
Fixed.
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.
On 2009/07/03 04:23:38, scottb wrote:
> There's also a core.client.impl package, would that be more
appropriate?
Moved.
http://gwt-code-reviews.appspot.com/46801/diff/1041/39#newcode29
Line 29: @Retention(RetentionPolicy.SOURCE)
On 2009/07/03 04:23:38, scottb wrote:
> Is this going to jive with IHM? I could foresee needed classfile
retention.
Fixed.
http://gwt-code-reviews.appspot.com/46801/diff/1041/39#newcode40
Line 40: String className();
Class literals have language access restrictions. The original version
did use class literals, but you can't legally to package-protected inner
types from code outside the package. Then you'd wind up with rescuer
types all over the place, instead of being able to generate exactly one
rescuer per RemoteService (making manual verification of rescued types
easier).
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-revi
[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
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(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
[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
On 2009/06/29 13:40:16, bobv wrote: > Ping Ping. http://gwt-code-reviews.appspot.com/46801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add ArtificialRescue annotation to compiler
Ping http://gwt-code-reviews.appspot.com/46801 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
