http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode312
dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:312:
// prune all Object.getClass() overrides and replace with inline field
ref
On 2011/06/10 14:54:40, jbrosenberg wrote:
It's a shame this can't be called earlier on, so that optimizers can
take
advantage of the loss of polymorphism with getClass()...
The Replace pass essentially does what the optimizers would do anyway,
which is inline the reference, but yeah, it would be nice.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode937
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:937: }
On 2011/06/10 14:54:40, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode246
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:246:
assert jsprogram.getFragmentCount() == 1;
On 2011/06/13 19:35:25, jbrosenberg wrote:
extra indentation, here and several places below in this method
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode261
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:261:
stat = result;
On 2011/06/13 19:35:25, jbrosenberg wrote:
this assignment is redundant, we have the same assignment below under
the "if
(keepIt)" block below.
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode345
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:345: */
On 2011/06/13 19:35:25, jbrosenberg wrote:
It looks like all this does is maybe remove some ctors (and if any
ctors remain,
set a flag). Would a better name be
"maybeRemoveCtorsFromDefineSeedStmt()"?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode357
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:357:
JConstructor ctor = (JConstructor) maybeCtor;
On 2011/06/13 19:35:25, jbrosenberg wrote:
how can ctor be null here?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#newcode504
dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:504:
JsNameRef lhs = (JsNameRef) binExpr.getArg1();
Not sure actually, I think this change came originally from scott a long
time ago who had merged something into my patch.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Is there a reason for the change in the way of testing that we are
using the "_"
tempVar. E.g. why no longer do:
if (!lhs.getName().getShortIdent().equals("_"))
Is it just for brevity? Or is there a requirement that the underBar
be existing
at this point (and is this a new requirement?).
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(left):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#oldcode1255
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1255:
On 2011/06/13 19:35:25, jbrosenberg wrote:
why get rid of this comment?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode610
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:610:
On 2011/06/13 19:35:25, jbrosenberg wrote:
white space
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode724
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:724:
Yes, because immortal types are hoisted to the top, and have no clinits
called. If not for the early exit, the rest of the method would add a
clinit call for to setup fields what have JSO initializers
(object/array), e.g. x = JavaScriptObejct.createObject() will produce a
clinit.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Are these checks needed here, since the corresponding
visit(JClassType...)
method returns false if x is the class literal holder, or is an
immortal code
gen type.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1438
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1438:
if (program.immortalCodeGenTypes.contains(x)) {
On 2011/06/13 19:35:25, jbrosenberg wrote:
s/Handle/Handled
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1665
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1665:
}
On 2011/06/13 19:35:25, jbrosenberg wrote:
Suggest change to return a new empty JsLiteralObject here, if castMap
is null
(since this is what all callers must do anyway).
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1816
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1816:
for (JMethod method : x.getMethods()) {
IIRC, I did this because the compiler inserts a synthesized constructor
even if none are defined.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Should this maybe be an assertion, that no immortal types should be
allowed to
have virtual methods or constructors? Otherwise it might be confusing
if they
are routinely thrown out? Should there also be a comment, similar to
the one in
JProgram where immortal gen types are defined, stating that immortal
types
should not have any non-static methods or fields?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1821
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1821:
if (JProgram.isClinit(method)) {
I'll add the assert. The reason why the clinit needs to exist is because
any references to an immortal type produce a call to the clinit at the
callsite, e.g. SeedUtil.newSeed(1) => (clinit(), newSeed(1)). So we just
output the clinit and let it get pruned later. I think it should be
empty, unless a clinit() calls a clinit(), which shouldn't occur I
think.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Why do we need to generate an empty clinit? Is it guaranteed to be
empty since
we don't allow any non-static fields? Add an assert that method is
empty and
contains no statements? Add a comment?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1837
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1837:
for (JField field : x.getFields()) {
On 2011/06/13 19:35:25, jbrosenberg wrote:
Assertion instead?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1845
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1845:
JExpression init = field.getInitializer();
On 2011/06/13 19:35:25, jbrosenberg wrote:
Comment here, explaining that immortal types can only have non literal
initializers for jso's that are instances of createArray or
createObject, etc.
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1848
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1848:
// no literal, but it could be a JavaScriptObject
If it were a primitive, then the JsVar would already have the correct
initializer (from being visited), and field.getLiteralInitializer()
would have returned other than null.
On 2011/06/13 19:35:25, jbrosenberg wrote:
If it's not a JSO, do we allow any initializer method? I believe in
the comment
in JProgram, it says that the initializer can be primitive, a literal,
or one of
the 2 JSO.create methods below. Should there be an else check that
it's a
primitive initializer here?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1904
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1904:
JsExpression castMap = generateCastableTypeMap(x);
On 2011/06/13 19:35:25, jbrosenberg wrote:
can we just have generateCastableTypeMap() return a new
JsObjectLiteral if it's
null, instead of handle here?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode2014
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:2014:
On 2011/06/13 19:35:25, jbrosenberg wrote:
white space
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
File
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java#newcode146
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java:146:
* <pre>
On 2011/06/13 19:35:25, jbrosenberg wrote:
s/JSeedIdOff/JSeedIdOf
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java#newcode267
dev/core/src/com/google/gwt/dev/jjs/impl/ImplementClassLiteralsAsFields.java:267:
private void execImpl() {
Left over comment from an older implementation that didn't leave the
getClass(), but simply omitted them. (This required generating calls to
implement class literals that were not directly referenced, but had
associated instantiated types)
On 2011/06/13 19:35:25, jbrosenberg wrote:
first? then what? Consider removing "first" in the comment
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
File dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java#newcode633
dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java:633: /**
On 2011/06/13 19:35:25, jbrosenberg wrote:
Update comment, since this is used for both regular codeGenTypes and
immortal
types. Consider changing the name of this method to "traverseTypes",
etc.
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java
File
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java#newcode25
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceGetClassOverrides.java:25:
Done. String.class is handled in Class::setClassLiteral, it pokes the
required class literal into String.prototype.clazz.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Need trailing '.'
Also, add explanation for special handling for JSO's. E.g. "Prune all
overrides
of Object.getClass(), except for when the enclosing class is
JavaScriptObject.
(and explain why?). How do instances of String.getClass() get
handled?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java
File
dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java#newcode65
dev/core/src/com/google/gwt/dev/js/JsSourceGenerationVisitorWithSizeBreakdown.java:65:
}
On 2011/06/13 19:35:25, jbrosenberg wrote:
white space
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java
File dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java#newcode584
dev/core/src/com/google/gwt/dev/js/JsStackEmulator.java:584: @Override
I can't remember how it got like this, but you're right, in fact, it can
just be part of endVisit.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Shouldn't this be visit(JsFunction,...)?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java
File dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java#newcode883
dev/core/src/com/google/gwt/dev/js/JsToStringGenerationVisitor.java:883:
}
On 2011/06/13 19:35:25, jbrosenberg wrote:
whitespace
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java
File dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java#newcode20
dev/core/src/com/google/gwt/dev/js/ast/JsSeedIdOf.java:20: /**
It differs from JsNameOf in that JsNameOf is used by deRPC to get the
shortname, whereas JsSeedId gives you the numeric ID of the type.
On 2011/06/13 19:35:25, jbrosenberg wrote:
Is this the full description? How is it different from JsNameOf?
Just a
placeholder for the special seedId?
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java
File
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode29
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:29:
On 2011/06/13 19:35:25, jbrosenberg wrote:
reformat line lengths here!
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode40
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:40:
public static native JavaScriptObject defineSeed(int id, int superSeed,
JavaScriptObject castableTypeMap) /*-{
On 2011/06/13 19:35:25, jbrosenberg wrote:
It looks like clazz is unused here
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java#newcode41
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/SeedUtil.java:41:
var seed = @com.google.gwt.lang.SeedUtil::seedTable[id], clazz;
On 2011/06/13 19:35:25, jbrosenberg wrote:
move this comment inside if clause
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java
File user/super/com/google/gwt/emul/java/lang/Class.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode45
user/super/com/google/gwt/emul/java/lang/Class.java:45: Class<T> clazz =
new Class<T>();
On 2011/06/13 19:35:25, jbrosenberg wrote:
shouldn't this call setName with seedId (and not -1)?
No, because there's no seed function for arrays, so no prototype to
lookup. The .clazz field is instead set by Array.initValues() which is
ultimately called by ArrayNormalizer. Array is a special case, like
String, where the 'clazz' field is not looked up by integer.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode147
user/super/com/google/gwt/emul/java/lang/Class.java:147: if (seedId >
-1) {
On 2011/06/13 19:35:25, jbrosenberg wrote:
s/pruned b using/pruned by using?
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Class.java#newcode211
user/super/com/google/gwt/emul/java/lang/Class.java:211:
On 2011/06/13 19:35:25, jbrosenberg wrote:
white space
Done.
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Object.java
File user/super/com/google/gwt/emul/java/lang/Object.java (right):
http://gwt-code-reviews.appspot.com/1447821/diff/3001/user/super/com/google/gwt/emul/java/lang/Object.java#newcode30
user/super/com/google/gwt/emul/java/lang/Object.java:30: */
On 2011/06/13 19:35:25, jbrosenberg wrote:
Is this not true for other fields in this class?
I'm not sure, but it appears to be a very special case of a field named
'clazz' conflicting with a param named 'clazz'. :(
http://gwt-code-reviews.appspot.com/1447821/diff/6001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
(right):
http://gwt-code-reviews.appspot.com/1447821/diff/6001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1824
dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1824:
assert ((JMethodBody) method.getBody()).getStatements().isEmpty() :
I had to remove this assert, since the clinits of immortable types can
be non-empty, that is, if B is an immortable type extending A which is
also an immortable type, then B's clinit will be non-empty and invoke
A's clinit (which is empty). This all gets cleaned up later by the
optimizer.
http://gwt-code-reviews.appspot.com/1447821/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
