Should be fixed now (weird it didn't pick up changes)
http://gwt-code-reviews.appspot.com/1336802/diff/1/4 File dev/core/src/com/google/gwt/dev/js/JsStringInterner.java (right): http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode48 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:48: /** On 2011/02/03 18:25:40, jbrosenberg wrote:
Should this comment be changed, to indicate that it can conditionally
intern
strings, based on occurrence count etc.?
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode69 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:69: * Prevents 'fixing' an otherwise illegal operation. On 2011/02/03 02:34:05, scottb wrote:
Are these actually useful in practice?
Dunno, they are copied from Bob's code below, so I assume he must have added them for good reason. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode74 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:74: || !(x.getArg1() instanceof JsStringLiteral); On 2011/02/03 02:34:05, scottb wrote:
You still need to visit the RHS. (You can do so explicitly.)
Bob's visitor below that does the replacement has the same logic, so I was trying to make my counter mirror existing the string literals he was visiting, so if I change this, I'll have to change that logic below, and that'll require some tests :) Leaving it as it is in the counter won't cause any harm. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode93 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:93: /** On 2011/02/03 18:25:40, jbrosenberg wrote:
Keep comment within 80 chars? Here and elsewhere...
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode99 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:99: return false; On 2011/02/03 02:34:05, scottb wrote:
Visit the RHS?
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode102 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:102: /** On 2011/02/03 18:25:40, jbrosenberg wrote:
Is this the right comment here?
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode232 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:232: Integer occurences = occurenceMap.get(x.getValue()); On 2011/02/03 18:25:40, jbrosenberg wrote:
Should the number '2' be a configurable number, instead of hard-coded
here?
What's special about 2?
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode233 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:233: if (occurences != null && occurences < 2) { On 2011/02/03 02:34:05, scottb wrote:
assert occurrences != null (something is broken if it is)
Done. http://gwt-code-reviews.appspot.com/1336802/diff/1/4#newcode362 dev/core/src/com/google/gwt/dev/js/JsStringInterner.java:362: private static Map<String, Integer> getOccurenceMap(JsNode node) { On 2011/02/03 02:34:05, scottb wrote:
How about "make" or "build" since it's an expensive op?
Done. http://gwt-code-reviews.appspot.com/1336802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
