On Fri, Apr 17, 2009 at 1:57 PM, Lex Spoon <[email protected]> wrote:
> One question: I see a change to how enums are created in
> GenerateJavaAST. What's going on there? I see code being deleted but
> no associated add.
>
I'm having trouble finding this. What's the approximate line number in
GenerateJavaAST of the change?
> Otherwise, LVGTM. I've been wanting this type hierarchy for a while
> just for logical cleanliness. I hadn't thought about the memory and
> speed implications, but those are excellent as well.
>
Sweet. :)
> Comments:
>
> JReferenceType has a commented-out getMethods() in the version I looked at.
>
Good catch, I left a dingleberry there.
>
> Logically, a little bit more could go down into JDeclaredType. They seem
> like good todos to mark, because ideally these questions wouldn't be asked
> on a general reference type. This issue becomes more important the more
> we expand the JReferenceType hierarchy.
>
Agreed.
> - superclass: arrays and null don't really have a superclass, so it's
> tricky to ask for it. Allowin callers to do so seems like an
> invitation for bugs,
> e.g. asking for the superclass of String[] when what was intended was
> asking
> for the superclass of String.
>
I did have a theoretical reason for leaving it there, because String[] does
actually have a super class, namely Object. But you could also call
this laziness. I got burned a couple of times trying to fiddle with super
class (kept having to revert), and finally gave up for now. In particular,
I didn't want to rework JProgram.generalizeTypes() and a couple of other
cases.
> - hasClinit: this question is only asked for field references, and a
> field always has a real enclosing class. The one exception I can
> think of the "length" field of an array; if that's the issue, maybe we
> should
> think about encoding array.length a different way, anyway.
>
Fixed. The "length" field is actually in a real class,
com.google.gwt.lang.Array. It's slightly counterintuitive but it beats
having a separate copy on every possible array type.
> Methods like removeMethod(int) are an improvement over directly accessing
> the
> list, but it looks fragile to me due to how careful the code has to be
> with the index
> passed into it. It would be a little simpler and safer to have a
> method like removeAllMethodsSuchThat(Predicate<JMethod>). For
> example, instead of this:
>
> for (int i = 1; i < type.getMethods().size(); ++i) {
> JMethod method = type.getMethods().get(i);
> // all other interface methods are instance and abstract
> if (!isInstantiated || !methodIsReferenced(method)) {
> type.removeMethod(i);
> didChange = true;
> --i;
> }
> }
>
> we could write this:
>
> didChange |= type.removeAllMethodsSuchThat(new Predicate<JMethod>() {
> public boolean apply(JMethod method) {
> // all other interface methods are instance and abstract
> return !isInstantiated || !methodIsReferenced(method);
> }
> }
>
> Just a thought. Sometimes this idiom can push out just a little
> further the complexity threshold where brains start melting.
>
Good thought. It might be worth doing this at some point if we end up with
more callers to the removal methods than just Pruner.
Okay, I tacked on one more commit to my reftypes branch with the corrections
noted above; you can re-fetch that branch if you'd like to double check me
(should be pretty straighforward tho, so it's not imperative).
Lemme know about GenerateJavaAST.
Thanks!
Scott
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
