LGTM w/nits (which you can ignore)

http://gwt-code-reviews.appspot.com/1453804/diff/1017/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/1453804/diff/1017/user/super/com/google/gwt/emul/java/lang/Object.java#newcode45
user/super/com/google/gwt/emul/java/lang/Object.java:45: /**
Well, typerMarker is actually set as part of the Object.prototype, and
all other prototypes (except String) inherit from it.  So, yeah, that's
already there.

I agree, there is probably a better way to refactor the functionality
embodied by the typeMarker field.  I think historically it was used for
more things (thus the strange name "typeMarker" which doesn't seem to
relate to it's current use at all).  The main reason it's not trivial to
remove altogether is because it is useful for detecting the GWT module
that an object belongs to (otherwise, it really isn't needed).

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
File dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
(right):

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java#newcode439
dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java:439: *
javadoc style {@link ...} syntax?  Or @see ...?

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java#newcode446
dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java:446:
JMethod method =
program.getIndexedMethod("Cast.throwClassCastExceptionUnlessNull");
Hmm....I think there's more to it than that.   It just wouldn't be
correct to implicitly upcast the expression to be of type Object (even
if it's a null value).  The "magic" is to make the method return a type
JNullType, which is "compatible" with an expression of any type that is
guaranteed to be null valued (since null can be cast to any type).  We
can't force the type to the original type of the cast, because that type
info is lost at this point (since it was overwritten to JNullType).

The reason the source method "throwClassCastExceptionUnlessNull" is
defined with a return type of Object in Cast.java, is because it needs
to be valid java (no JNullType in java).

How about:

"Note, we must update the type of this method call to return the null
type."

http://gwt-code-reviews.appspot.com/1453804/diff/8001/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/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode377
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:377: *
How about (just quibbling here):

"Code flow analysis is run separately on methods which implement
RunAsyncCallback.onSuccess(), as top-level entry points."

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
(right):

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#newcode65
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:65: /*
".... to implementations of RunAsyncCallback.onSuccess()...."

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
File dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
(right):

http://gwt-code-reviews.appspot.com/1453804/diff/8001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java#newcode346
dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java:346: *
Exception.
"See {@link CastNormalizer}."

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

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

Reply via email to