On Thu, Apr 21, 2011 at 6:54 PM, <j...@google.com> wrote: > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java > File user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java > (right): > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode60 > user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:60: > JType... argTypes) { > On 2011/04/22 01:13:17, rjrjr wrote: > >> I don't know that I would ever use this unless it was based on >> JClassType.getOverridableMethods(). >> > > But rather than hardcoding that choice, perhaps you should make the >> > first > >> argument a JMethod[] to let me choose the set myself? Can put @see >> > pointing to > >> getOverridableMethods. >> > > Ok, how about a method taking the method list and one taking the type, > which defaults to either getMethods or getOverridableMethods (whichever > you think is more useful). I would think in this application it is most > interesting in this context, since you want to know if you can call the > method with a set of parameters, and whether it is implemented on that > class or a superclass is just an implementation detail.
Sounds good. Definitely getOverridableMethods. > > > I'm biting my tongue about the fact that this method is unused. I'm >> > generally > >> opposed to speculative coding, but I can imagine cleaning up some >> > existing code > >> with this. >> > > It just seemed an obvious extension of the constructor code, but it is > certainly easy enough to leave it out if you prefer. Nah, I wants it. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode65 > user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:65: if > (method.isStatic() == isStatic && methodName.equals(method.getName()) > On 2011/04/22 01:13:17, rjrjr wrote: > >> could you put () around the == statement? >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode78 > user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:78: * > @param argType > On 2011/04/22 01:13:17, rjrjr wrote: > >> What's the difference between a paramType and an argType? Seems like >> > the > >> distinction matters here, but param and arg look like synonyms to me. >> > > paramType is the type of the formal parameter declaration, while argType > is the type of the argument to be passed in. > > > Should the args be leftHandType and rightHandType? This applies >> > throughout the > >> class of course, not just here. >> > > That would work, although the terminology seems a bit odd in the context > of a method call. I guess param and arg make sense. I've never decoupled them before. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode140 > user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:140: > JType[] ctorArgTypes = paramTypes; > On 2011/04/22 01:13:17, rjrjr wrote: > >> Looks like ctorArgTypes is refactoring chaff, should be inlined. >> > > Yes, I created it just so I could inline it, then never did :). Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java#newcode157 > user/src/com/google/gwt/uibinder/rebind/TypeOracleUtils.java:157: } else > { > On 2011/04/22 01:13:17, rjrjr wrote: > >> else if >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java > File user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java > (right): > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode29 > user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:29: > On 2011/04/22 01:13:17, rjrjr wrote: > >> import static com.google.gwt.uibinder.rebind.TypeOracleUtils.*; might >> > make this > >> easier to read. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode145 > user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:145: > public void testHasCompatibleMethod() { > On 2011/04/22 01:13:17, rjrjr wrote: > >> Many lines too long. Did you auto format? >> > > Yes, although I did make some edits since then. The @link tags can't be > split, and Reitveld is wrapping at 80 instead of 100. I will make sure > it is formatted properly before submitting. I have Rietveld set to 100. There's a setting for that. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode146 > user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:146: > assertTrue("didn't find static m1(Child)", > TypeOracleUtils.hasCompatibleMethod(foo, true, "m1", > On 2011/04/22 01:13:17, rjrjr wrote: > >> s/didn't find/Should have found/ >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode148 > user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:148: > assertFalse("found m1(Child)", TypeOracleUtils.hasCompatibleMethod(foo, > false, "m1", child)); > On 2011/04/22 01:13:17, rjrjr wrote: > >> s/found/Should not have found/ >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1420808/diff/1/user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java#newcode215 > user/test/com/google/gwt/uibinder/rebind/TypeOracleUtilsTest.java:215: > protected void setUp() throws Exception { > On 2011/04/22 01:13:17, rjrjr wrote: > >> please throw specific exception types >> > > This was just left by the automatic stub generation in Eclipse -- I'll > tighten it down. > > > http://gwt-code-reviews.appspot.com/1420808/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
