[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
I am noticing that while unnecessary casts of an EnumType to (Enum) no longer happen, there is still a cast generated, for the EnumType itself. Is that necessary? E.g., I'm seeing code like this: Fruit fruit = Fruit.APPLE; int i = fruit.ordinal(); ending up in the AST like this (from an AST dump): EntryPoint$Fruit fruit = EntryPoint$Fruit.APPLE; int i = ((EntryPoint$Fruit) fruit).ordinal; Is that extra cast necessary still? I don't think it affects correctness, and I assume it gets optimized away later, but I wonder if we can prevent it from getting inserted at all here. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
On 2011/04/27 15:12:57, jbrosenberg wrote:
I am noticing that while unnecessary casts of an EnumType to (Enum) no
longer
happen, there is still a cast generated, for the EnumType itself. Is
that
necessary?
E.g., I'm seeing code like this:
Fruit fruit = Fruit.APPLE;
int i = fruit.ordinal();
ending up in the AST like this (from an AST dump):
EntryPoint$Fruit fruit = EntryPoint$Fruit.APPLE;
int i = ((EntryPoint$Fruit) fruit).ordinal;
Is that extra cast necessary still? I don't think it affects
correctness, and I
assume it gets optimized away later, but I wonder if we can prevent it
from
getting inserted at all here.
That cast is the result of the new 'merge' operation, basically. It's
not clear from the AST dump, but the cast is actually a non-null cast.
'fruit' is a nullable Fruit, but 'this$static' which it is replacing is
a non-null Enum. The merge operation turns the cast into a non-null
Fruit. Arguably, that's an irrelevant detail that we should forget
about. Also arguably, MakeCallStatic might be doing the wrong thing
anyway in making this$static non-null; the implicit guarantee of
non-nullity gets lost once you make the call static. For now, I think
it's ok.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode646
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:646: *
qualifying instance.
Actually, crap, I'm going to have to revert this. Because there is no
relationship between when a JVariableRef and JVariable (for example) get
visited, we can't do it in one pass. We can't rely on instance type
being updated already, and we can't rely on it NOT being update, either,
since it will be computed dynamically from the referenced Variable,
which may or may not have been updated.
I need to revert my change that merges them.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode653
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:653:
On 2011/04/27 04:01:55, jbrosenberg wrote:
Can this comment be worded a bit more clearly? How about:
Replace any references to an enum ordinal field with the qualifying
instance,
if that instance has been ordinalized (e.g. change
code4.ordinal/code with
code4/code).
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode659
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:659:
public void endVisit(JFieldRef x, Context ctx) {
Yes, in a later patch, TypeRemapper does need to implement
endVisit(JMethodCall).
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode670
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:670:
On 2011/04/27 04:01:55, jbrosenberg wrote:
ditto
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode793
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:793:
++removeIndex;
On 2011/04/27 04:01:55, jbrosenberg wrote:
Feel free, looks good. Maybe also add in the tests I had for testing
an
empty/unused enum.
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java (right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java#newcode69
dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java:69:
x.setType(modRemap(x.getType()));
I see what you're saying, I'll change the comment to reflect this.
Actually, JGwtCreate's sourceType and resultTypes should really be in
terms of String rather than JType, for all it's used for. The
underlying instantiation expressions get mapped ok. I may roll another
CL for this.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
File dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java#newcode86
dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java:86: public
CharSequence getContent() {
On 2011/04/27 04:01:55, jbrosenberg wrote:
This comment no longer applies?
Done.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(right):
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
New (hopefully final?) patch. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
Updated based on feedback. http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java (left): http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#oldcode865 dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:865: } Without the whole 'ignored cast operations' this part isn't needed. http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#oldcode902 dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:902: } Ditto. http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java (right): http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode646 dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:646: * qualifying instance. Merged in the other visitor. http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode793 dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:793: ++removeIndex; Went ahead and merged in your change too. I can revert this part if you'd rather submit yours. http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java File dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java (right): http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java#newcode101 dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java:101: return result; Implemented the madeChanges() as you mentioned. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode444
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:444:
maybe add a test for this case in EnumOrdinalizerTest
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode646
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:646: *
qualifying instance.
This is ok, I think, since it is safe to assume that the
endVisit(JFieldRef,...) and endVisit(JMethodCall,..) methods below will
always be called after their instance expressions have been visited by
TypeRemapper, or by one of the visit() methods below. This seems born
out by JFieldRef.traverse() and JMethodCall.traverse()Make sense?
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode653
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:653:
Can this comment be worded a bit more clearly? How about:
Replace any references to an enum ordinal field with the qualifying
instance, if that instance has been ordinalized (e.g. change
code4.ordinal/code with code4/code).
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode659
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:659:
public void endVisit(JFieldRef x, Context ctx) {
This super method call won't actually do anything in this case, but I
assume you are adding it for future code-safety, should TypeRemapper
choose to override it later? (same for the endVisit(JMethodCall,...)
case below).
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode670
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:670:
ditto
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode676
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:676:
public void endVisit(JMethodCall x, Context ctx) {
ditto
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java#newcode793
dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java:793:
++removeIndex;
Feel free, looks good. Maybe also add in the tests I had for testing an
empty/unused enum.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java (right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java#newcode69
dev/core/src/com/google/gwt/dev/jjs/impl/TypeRemapper.java:69:
x.setType(modRemap(x.getType()));
Hmmm, I'm not sure this should apply to x.getResultTypes(). For a
particular permutation, I would think the remapping would apply equally
to generated code, and so if one of the resultTypes needs to be remapped
it would happen after the generators have run for a particular
permutation.
If it's called prior to the ReplaceRebinds visitor, it would be
necessary to change mappings for which type to generate for which
permutation, and this implies affecting the generate-with rules, etc.
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
File dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java#newcode86
dev/core/test/com/google/gwt/dev/jjs/JavaAstConstructor.java:86: public
CharSequence getContent() {
This comment no longer applies?
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
File dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java
(right):
http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java#newcode22
dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java:22:
/**
add tests for enums used in an instanceof, and for empty enums. can
leave as a TODO and I can take a look later.
http://gwt-code-reviews.appspot.com/1426804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: EnumOrdinalizer cleanup (issue1426804)
LGTM + nits (I've run tests on some big compiles, and ordinalization results look good) http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java File dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java (right): http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java#newcode195 dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java:195: /* This comment is no longer true, and can be removed http://gwt-code-reviews.appspot.com/1426804/diff/3002/dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java#newcode671 dev/core/test/com/google/gwt/dev/jjs/impl/EnumOrdinalizerTest.java:671: }); Is there a reason for this change? I've tested with and without this change, and tests pass. http://gwt-code-reviews.appspot.com/1426804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
