[gwt-contrib] Re: code review requested - allow OpenJDK to compile GWT
On Mon, Apr 6, 2009 at 9:07 AM, BobV b...@google.com wrote: Here is the patch which does what Scott suggested. Any objections? LGTM Thanks, committed with Scott's javadoc change at r5184. -- John A. Tamplin Software Engineer (GWT), Google --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: code review requested - allow OpenJDK to compile GWT
On Sat, Mar 28, 2009 at 11:55 PM, Scott Blum sco...@google.com wrote:
Two things:
1) Do you have to use a temp, or can you do something
like artifacts. EmittedArtifact find(EmittedArtifact.class)? Or whatever
the syntax is.
It could also be:
for (EmittedArtifact artifact : artifacts.EmittedArtifact,
EmittedArtifactfind(EmittedArtifact.class)) {
etc if you prefer -- do you think that is cleaner? The problem here is that
OpenJDK can't seem to properly infer A=EmittedArtifact from the return type
SortedSetA extends Artifact? where EmittedArtifact extends
ArtifactEmittedArtifact (defined as ArtifactC extends ArtifactC).
This is just one case of a general class of recursive definitions which
OpenJDK fails to infer, and the Sun guy working on it has determined that
the old javac accepted in violation of JLS. Since this seems very similar
to the definition of Enum, I can only guess Enum is special-cased.
Still another option would be to pass an explicit type token for the return
type. We currently do not use the ability to return a different type, so
this would not require changing the call-sites if we did this:
public T extends Artifact? super T SortedSetT find(ClassT
artifactType) {
return find(artifactType, artifactType);
}
public A extends Artifact? super A, T extends Artifact? super T
SortedSetA find(ClassA returnType, ClassT artifactType) {
...
}
Would that be preferable? It would make it harder to introduce something
that wouldn't compile on OpenJDK, as you would get a compile error and would
have to add a second type token if you tried to return a different type, and
it avoids having to infer the return type in all current uses.
2) What's the plan for preventing regressuions or new occurrences of this in
the future?
I think when we upgrade our continuous build system, we should build with
OpenJDK. Until then, I can build with OpenJDK so whenever I build trunk it
would be noticed.
--
John A. Tamplin
Software Engineer (GWT), Google
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: code review requested - allow OpenJDK to compile GWT
On Mon, Mar 30, 2009 at 1:57 PM, Scott Blum sco...@google.com wrote:
John and I discussed this face to face. It turns out that the find()
method has, perhaps, an unnecessarily complicated specification that pushes
generic compile time sugar arguably past the point of usefulness --- and
definitely past what OpenJDK allows.
So our tentative resolution is to simplify the declaration of find() and
make it slightly less powerful. It turns out none of our own code is making
any use whatsoever of the extra flexibility it currently has.
Here is the patch which does what Scott suggested. Any objections?
--
John A. Tamplin
Software Engineer (GWT), Google
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
Index: dev/core/src/com/google/gwt/core/ext/linker/ArtifactSet.java
===
--- dev/core/src/com/google/gwt/core/ext/linker/ArtifactSet.java (revision 5074)
+++ dev/core/src/com/google/gwt/core/ext/linker/ArtifactSet.java (working copy)
@@ -61,6 +61,7 @@
return treeSet.containsAll(c);
}
+ @Override
public boolean equals(Object o) {
return treeSet.equals(o);
}
@@ -68,28 +69,29 @@
/**
* Find all Artifacts assignable to some base type. The returned value will be
* a snapshot of the values in the ArtifactSet. The following two examples
- * result in an equivalent set:
+ * result in an equivalent set (assuming there were no EmittedArtifacts
+ * present not of type PublicResource or GeneratedResource):
*
* pre
- * SortedSetlt;EmittedArtifactgt; search = artifactSet.find(PublicResource.class);
- * search.addAll(artifactSet.find(GeneratedResource.class);
+ * SortedSetlt;EmittedArtifactgt; search = new TreeSetlt;EmittedArtifactgt;();
+ * search.addAll(artifactSet.find(PublicResource.class));
+ * search.addAll(artifactSet.find(GeneratedResource.class));
* /pre
*
* or
*
* pre
- * SortedSetlt;EmittedArtifactgt; search = artifactSet.find(EmittedArtifact.class);
+ * SortedSetlt;EmittedArtifactgt; search = artifactSet.find(EmittedArtifact.class);
* /pre
*
- * @param A a type bound possibly wider than the desired type of artifact
* @param T the desired type of Artifact
* @param artifactType the desired type of Artifact
* @return all Artifacts in the ArtifactSet assignable to the desired type
*/
- public A extends Artifact?, T extends A SortedSetA find(
+ public T extends Artifact? super T SortedSetT find(
ClassT artifactType) {
// TODO make this sub-linear (but must retain order for styles/scripts!)
-SortedSetA toReturn = new TreeSetA();
+SortedSetT toReturn = new TreeSetT();
for (Artifact? artifact : this) {
if (artifactType.isInstance(artifact)) {
toReturn.add(artifactType.cast(artifact));
@@ -113,6 +115,7 @@
}
}
+ @Override
public int hashCode() {
return treeSet.hashCode();
}
[gwt-contrib] Re: code review requested - allow OpenJDK to compile GWT
I think the javadoc is now more confusing than useful, due to the weird caveat about public/generated resources. How about we just kill all the javadoc from The following two examples... up to the @param list? Or else, give the most useful and common example, using a for-each loop with the result of a find(). On Mon, Mar 30, 2009 at 2:33 PM, John Tamplin j...@google.com wrote: On Mon, Mar 30, 2009 at 1:57 PM, Scott Blum sco...@google.com wrote: John and I discussed this face to face. It turns out that the find() method has, perhaps, an unnecessarily complicated specification that pushes generic compile time sugar arguably past the point of usefulness --- and definitely past what OpenJDK allows. So our tentative resolution is to simplify the declaration of find() and make it slightly less powerful. It turns out none of our own code is making any use whatsoever of the extra flexibility it currently has. Here is the patch which does what Scott suggested. Any objections? -- John A. Tamplin Software Engineer (GWT), Google --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: code review requested - allow OpenJDK to compile GWT
Two things: 1) Do you have to use a temp, or can you do something like artifacts. EmittedArtifact find(EmittedArtifact.class)? Or whatever the syntax is. 2) What's the plan for preventing regressions or new occurrences of this in the future? On Sat, Mar 28, 2009 at 11:19 PM, John Tamplin j...@google.com wrote: OpenJDK has some issues inferring generic types in some cases. There was some attempt to address it http://markmail.org/message/cen3lghawfnnxxgp, but it turned out to be a partial fix and there was some concern that the older javac was improperly allowing some of these constructs. External users are starting to run into this as newer Linux distros are moving to OpenJDK for the default Java install, so I think we should go ahead and fix it in our code base. The attached patch, relative to trunk r5098, fixes this by adding temporary variables to avoid the bug. -- John A. Tamplin Software Engineer (GWT), Google --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
