The implementation looks good, and the new feature is very good.  I
have two design questions, though:

1. Is there any reason to store all correlates as strings?  The
problem with strings is that any code that wants to process the
correlates, as opposed to simply printing them out, will need to pare
the string and find the original object referred to.  This is surely a
performance drain, and also it is also error prone.  What would you
think of having method, field, and function correlates store
references to JMethods, JFields, and JsFunctions?  This would require
modifying the Correlate class, but it doesn't look too terrible.  This
could be done in a subsequent patch if you prefer, but I'd like to at
least make a plan.

2. Attaching a method to its overrider parent seems to mix two
separate meanings of ancestor.  In most of SOYC, the ancestors of a
node are previous versions of the same node, only before some
transformation had occurred.  In the case of method override, however,
the two methods are completely distinct.  You say that making one the
ancestor of the other means that you can find one method from the
other without needing a type oracle.  However, it turns out that
callers of the compiler are going to be able to get a type oracle,
anyway, once runAsync is merged in.  Given this, is there any further
reason to have method override imply ancestry?  It seems to dilute the
information that SourceInfo ancestry can uniquely provide.



On small notes:

- Why are there synchronized methods in SourceInfo?  In particular,
why are some synchronized but not others?  I would not have expected
this class to ever get used by multiple threads at the same time.  Any
particular JProgram or JsProgram should be owned by one thread at a
time, and ownership transfer always involves a synchronization point.

- It would be really nice to have utility methods that find and return
the primary and secondary correlates on a specified axis.  If I
understand correctly, a caller must currently search through the
return value of getCorrelations or getDerivedCorrelations.


Lex

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

Reply via email to