[gwt-contrib] Re: Have JsInliner refuse to interpose a new expression (issue344801)

2010-04-13 Thread spoon 

Thanks, Scott.

I added the TODO at the place indicated.  It applies more broadly,
though: any place the inliner looks at a function call, it would be nice
to know what if any side effects that function call has and would be
affected by.


http://gwt-code-reviews.appspot.com/344801/diff/1/2
File dev/core/src/com/google/gwt/dev/js/JsInliner.java (right):

http://gwt-code-reviews.appspot.com/344801/diff/1/2#newcode677
dev/core/src/com/google/gwt/dev/js/JsInliner.java:677: public void
endVisit(JsNew x, JsContext ctx) {
Done and done.

http://gwt-code-reviews.appspot.com/344801/diff/1/3
File dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java (right):

http://gwt-code-reviews.appspot.com/344801/diff/1/3#newcode120
dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java:120:
System.err.println("Input vs ");
On 2010/04/13 15:07:30, scottb wrote:

Unrelated to your patch, but maybe yank this line while you're here?


Done.

http://gwt-code-reviews.appspot.com/344801/show

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

[gwt-contrib] Re: Have JsInliner refuse to interpose a new expression (issue344801)

2010-04-13 Thread scottb 

Oh, I had one final thought on this, that might be worth a note for the
future, if my logic is sound:

You might imagine having special-cased JsInvocations to an empty target
method to not break evaluation order (since they do nothing).  As a
practical matter, however, we didn't bother doing so because such
constructs would have already been inlined by the time we get here.
This is NOT true for a JsNew op, which can never be inlined.  We might
want to add a TODO that a JsNew op with an empty target method could be
special-cased to not break EvalutationOrder.


http://gwt-code-reviews.appspot.com/344801/show

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

To unsubscribe, reply using "remove me" as the subject.

[gwt-contrib] Re: Have JsInliner refuse to interpose a new expression (issue344801)

2010-04-13 Thread scottb 

LGTM, nits.


http://gwt-code-reviews.appspot.com/344801/diff/1/2
File dev/core/src/com/google/gwt/dev/js/JsInliner.java (right):

http://gwt-code-reviews.appspot.com/344801/diff/1/2#newcode677
dev/core/src/com/google/gwt/dev/js/JsInliner.java:677: public void
endVisit(JsNew x, JsContext ctx) {
I assume the logic is essentially the same as for
endVisit(JsInvocation)?  Tiny bit of javadoc noting that would be good.
Also, I think this wants to sort down below endVisit(JsNameRef).

http://gwt-code-reviews.appspot.com/344801/diff/1/3
File dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java (right):

http://gwt-code-reviews.appspot.com/344801/diff/1/3#newcode120
dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java:120:
System.err.println("Input vs ");
Unrelated to your patch, but maybe yank this line while you're here?

http://gwt-code-reviews.appspot.com/344801/show

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

To unsubscribe, reply using "remove me" as the subject.

[gwt-contrib] Re: Have JsInliner refuse to interpose a new expression (issue344801)

2010-04-13 Thread spoon 

Can anyone familiar with GWT's optimization review this?

http://gwt-code-reviews.appspot.com/344801/show

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

To unsubscribe, reply using "remove me" as the subject.