High level, this seems like a good direction.  My only concern is that
this feels a little... haphazard.  I think as a future direction, we
need to figure out some kind of overall strategy for what gets interned,
and where.  Kind of like deciding on callee-save vs. caller-save
registers, or the AddRef/Release semantics on [in,out] vs. [out,retval]
in COM. :)



http://gwt-code-reviews.appspot.com/870801/diff/1/3
File dev/core/src/com/google/gwt/core/ext/typeinfo/JAbstractMethod.java
(right):

http://gwt-code-reviews.appspot.com/870801/diff/1/3#newcode245
dev/core/src/com/google/gwt/core/ext/typeinfo/JAbstractMethod.java:245:
return StringInterner.get().intern(realParameterName);
Your CL looks good, huge improvement, but I should mention this whole
param name thing is a huge mess.

http://gwt-code-reviews.appspot.com/870801/diff/1/9
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(left):

http://gwt-code-reviews.appspot.com/870801/diff/1/9#oldcode128
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:128:
return str;
yay!

http://gwt-code-reviews.appspot.com/870801/diff/1/9
File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
(right):

http://gwt-code-reviews.appspot.com/870801/diff/1/9#newcode91
dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:91:
unresolvedSimple.add(interner.intern(String.valueOf(simpleRef)));
Totally side band.... but if you have any brilliant ideas about how to
go from a char[] -> interned String -- without having to create the
intermediary string -- I'm all ears.

http://gwt-code-reviews.appspot.com/870801/diff/1/22
File
dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java
(right):

http://gwt-code-reviews.appspot.com/870801/diff/1/22#newcode165
dev/core/src/com/google/gwt/dev/resource/impl/ResourceOracleImpl.java:165:
f.getName().toLowerCase(Locale.ENGLISH));
Not useful, the String is only used locally.

http://gwt-code-reviews.appspot.com/870801/diff/1/23
File dev/core/src/com/google/gwt/dev/resource/impl/ZipFileResource.java
(right):

http://gwt-code-reviews.appspot.com/870801/diff/1/23#newcode53
dev/core/src/com/google/gwt/dev/resource/impl/ZipFileResource.java:53:
"jar:" + classPathEntry.getLocation() + "!/" + path);
This feels strange, since it's transient here.  What's the advantage to
interning?  IE: who holds a reference to the result?

http://gwt-code-reviews.appspot.com/870801/diff/1/24
File dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
(right):

http://gwt-code-reviews.appspot.com/870801/diff/1/24#newcode187
dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java:187: String
mangledName = StringInterner.get().intern(sb.toString());
Are both this change and the other one needed?  Seems like if you do
this one, the only other place you'd need to intern is the call to
field.getName().

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

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

Reply via email to