[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread zundel 

lgtm

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread scottb 


http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2837
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2837:
private static boolean isCompileTimeConstant(FieldBinding binding) {
You would think, yep.  It's just an assertion check on the fact that we
don't model final and volatile independently.  We use a single enum
value for either.

http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2840
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2840:
binding.isStatic() && binding.isFinal() && (binding.constant() !=
Constant.NotAConstant);
Doesn't matter, in byte code, the check for assertions being enabled is
itself an if test that is slower (as written) than the local variable
check.  If byte code optimizers are running, it should remove this
entirely.

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread jbrosenberg 

LGTM w/a couple questions


http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2837
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2837:
private static boolean isCompileTimeConstant(FieldBinding binding) {
I don't think it's legal/syntactically possible for a java field to be
both final and volatile, which would make this assertion tautologically
true, no?

http://gwt-code-reviews.appspot.com/1449818/diff/2003/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2840
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2840:
binding.isStatic() && binding.isFinal() && (binding.constant() !=
Constant.NotAConstant);
should this if test live inside the assert, maybe as a conditional, so
the if test doesn't get evaluated if assertions not enabled?

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread scottb 

Okay, much nicer! :D

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread scottb 

Yes, according to the JLS, Strings are considered compiled time
constants, but we've been doing this wrong up to now.  (Not that it
mattered before now.)


http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java
File dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java (right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java#newcode573
dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java:573:
This copy will get deleted soon, but the problem is getting a reference
to java.lang.String.  I'll see what I can do.

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java#newcode574
dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java:574: boolean
isCompileTimeConstant =
On 2011/06/14 23:46:25, jbrosenberg wrote:

extra parens in "(binding.isFinal())"


Done.

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2971
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2971:
boolean isCompileTimeConstant =
On 2011/06/14 23:46:25, jbrosenberg wrote:

extra parens in "(binding.isFinal())"


Done.

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode245
dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:245:
boolean isCompileTimeConstant =
On 2011/06/14 23:46:25, jbrosenberg wrote:

extra parens in "(binding.isFinal())"


Done.

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode246
dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:246:
binding.isStatic() && (binding.isFinal()) && (binding.constant() !=
Constant.NotAConstant);
Can't get a reference to java.lang.String from here; but I'll see what I
can do.

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread jbrosenberg 

Can you briefly describe the reason for this change?
If I understand correctly, the change is to extend handling of compile
time constants to Strings as well as base types?


http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java
File dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java (right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java#newcode573
dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java:573:
This same logic seems to be repeated in each file in this patch.  Can it
be made into a shared utility method somewhere?

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java#newcode574
dev/core/src/com/google/gwt/dev/jjs/impl/BuildTypeMap.java:574: boolean
isCompileTimeConstant =
extra parens in "(binding.isFinal())"

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
File dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java#newcode2971
dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java:2971:
boolean isCompileTimeConstant =
extra parens in "(binding.isFinal())"

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java
(right):

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode245
dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:245:
boolean isCompileTimeConstant =
extra parens in "(binding.isFinal())"

http://gwt-code-reviews.appspot.com/1449818/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java#newcode246
dev/core/src/com/google/gwt/dev/jjs/impl/ReferenceMapper.java:246:
binding.isStatic() && (binding.isFinal()) && (binding.constant() !=
Constant.NotAConstant);
why no assert here (you have it in similar places elsewhere)?

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: GwtAstBuilder better handling of JSNI refs to constants (issue1449818)

2011-06-14 Thread scottb 

Patch updated to fix GwtAstBuilderTest.

http://gwt-code-reviews.appspot.com/1449818/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors