I think it should be okay to commit. Committed as r3703. Thanks!
On Thu, Oct 2, 2008 at 5:17 PM, Scott Blum <[EMAIL PROTECTED]> wrote:
> Looks fine via glance over, but I didn't do a detailed review. Feel free
> to either commit or get a second opinion as you see fit.
>
>
> On Wed, Oct 1, 2008 at 6:17 PM, Amit Manjhi <[EMAIL PROTECTED]> wrote:
>
>> Hi Scott,
>>
>> Please review the attached patch which adds the conservative test for
>> api-checking that we talked about. I also cleaned up the code that removes
>> "duplicates" reported in Api incompatibilities. There are no significant
>> additions to white-list beyond the last patch.
>>
>> I carefully reviewed the code and patch and everything is functionally
>> correct. Sorry for taking so long to do this patch.
>>
>> Regards,
>> Amit
>>
>>
>> On Fri, Aug 15, 2008 at 6:40 PM, Scott Blum <[EMAIL PROTECTED]> wrote:
>>
>>> Yeah, that sounds rights. See if there are any methods in the new API
>>> such that a comparison to the old API yields no warnings. Otherwise, just
>>> pick the "closest" and report.
>>>
>>>
>>> On Thu, Aug 14, 2008 at 7:45 PM, Amit Manjhi <[EMAIL PROTECTED]>wrote:
>>>
>>>> [-gwtc]
>>>>
>>>> Hi Scott,
>>>>
>>>> Just to check that we are on the same page, by previous idea, I hope you
>>>> meant "check an existing API method with all compatible methods in the new
>>>> API, while accounting for easy-to-check exceptions" rather than "using JLS
>>>> to resolve non-overridable methods" because the latter can result in false
>>>> negatives, as in the example below. And as we discussed after pong, false
>>>> positives are okay, but false negatives are not.
>>>>
>>>> Thanks,
>>>> Amit
>>>>
>>>>
>>>> On Thu, Aug 14, 2008 at 5:32 PM, Scott Blum <[EMAIL PROTECTED]> wrote:
>>>>
>>>>> I think the previous idea is probably "good enough" as long as we don't
>>>>> change our APIs in pathological ways.
>>>>>
>>>>>
>>>>> On Thu, Aug 14, 2008 at 5:30 PM, Amit Manjhi <[EMAIL PROTECTED]>wrote:
>>>>>
>>>>>> Hi Scott,
>>>>>>
>>>>>> It seems the problem of finding the "best match" is quite complicated,
>>>>>> irrespective of whether the method is overridable or not. The key
>>>>>> observation is that the client code need not invoke the method with the
>>>>>> same
>>>>>> exact argument types -- it can invoke the method with any subclass types.
>>>>>> For example,
>>>>>>
>>>>>> class A { /* old version */
>>>>>> final void foo(Set<String> p1, Set<String> p2);
>>>>>> }
>>>>>>
>>>>>> class A { /* new version */
>>>>>> final void foo(Set<String> p1, Set<String> p2);
>>>>>> void foo(HashSet<String> p1, Set<String> p2) throws ...;
>>>>>> }
>>>>>>
>>>>>> In this case, a client code calling foo(HashSet<String>(),
>>>>>> Set<String>()) on object of type A will get a compile-time error. There
>>>>>> is
>>>>>> source-code incompatibility. So it seems a method of the old API must be
>>>>>> checked against all compatible methods of the new API for API
>>>>>> incompatibilities like changes in throws clause, incompatiblity in the
>>>>>> type
>>>>>> of return value, etc.
>>>>>>
>>>>>> However, as you can guess, this does not capture all cases. There are
>>>>>> exceptions. For example, if class A in old version had the same 2 methods
>>>>>> (with exact signatures) as class A in the new version, there is no API
>>>>>> incompatibility. I still need to think whether these exceptions can be
>>>>>> easily accounted for.
>>>>>>
>>>>>> Thoughts?
>>>>>> Amit
>>>>>>
>>>>>>
>>>>>> On Tue, Aug 12, 2008 at 10:43 PM, Amit Manjhi <[EMAIL PROTECTED]>wrote:
>>>>>>
>>>>>>> This approach seems reasonable. I will look into it.
>>>>>>>
>>>>>>> Amit
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Aug 12, 2008 at 10:17 PM, Scott Blum <[EMAIL PROTECTED]>wrote:
>>>>>>>
>>>>>>>> +tobyr
>>>>>>>> It might be good to discuss this face-to-face, but here's my
>>>>>>>> thinking:
>>>>>>>>
>>>>>>>> If the method is overridable, then you need a perfect match anyway,
>>>>>>>> so finding a 'best match' shouldn't be an issue.
>>>>>>>>
>>>>>>>> If the method is not overridable, then you only need to worry about
>>>>>>>> callers. In that case, here's my thinking on the potential best
>>>>>>>> approach:
>>>>>>>> take the parameter types of the old method and perform a Java Overload
>>>>>>>> Resolution according to the JLS as if you were calling into the class
>>>>>>>> with
>>>>>>>> those exactly typed arguments. If you fail to resolve an overload,
>>>>>>>> game
>>>>>>>> over. If you do successfully resolve, then perform the remaining
>>>>>>>> checks
>>>>>>>> against the resolved overload.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Aug 12, 2008 at 10:09 PM, Amit Manjhi <
>>>>>>>> [EMAIL PROTECTED]> wrote:
>>>>>>>>
>>>>>>>>> Hi Scott,
>>>>>>>>>
>>>>>>>>> After looking through the code again, I feel good about everything
>>>>>>>>> except the part in
>>>>>>>>> ApiClassDiffGenerator.java::processElementsInIntersection
>>>>>>>>> that determines the Api method in the new Api which is the best match
>>>>>>>>> for a
>>>>>>>>> Api method of the existing Api. There might be a case where multiple
>>>>>>>>> methods
>>>>>>>>> in the new Api might be "compatible with" a method in existing Api.
>>>>>>>>> Then,
>>>>>>>>> the api checker will flag an incompatibility with whichever method it
>>>>>>>>> analyzes first. I need to think more about this case. At the minimum,
>>>>>>>>> it
>>>>>>>>> seems I should ensure that the same new-api-method is selected in
>>>>>>>>> each run.
>>>>>>>>> However, this new code is a definite improvement over the previous
>>>>>>>>> code.
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Amit
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Aug 12, 2008 at 8:43 PM, Scott Blum <[EMAIL PROTECTED]>wrote:
>>>>>>>>>
>>>>>>>>>> Amit, sorry for sitting on this. I did a cursory check and it
>>>>>>>>>> seems reasonable.. is there anything you feel squirrelly about or
>>>>>>>>>> would like
>>>>>>>>>> me to scrutinize? Otherwise looks fine.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 6, 2008 at 12:59 AM, Amit Manjhi <
>>>>>>>>>> [EMAIL PROTECTED]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Scott,
>>>>>>>>>>>
>>>>>>>>>>> Please review the attached patch which addresses the following
>>>>>>>>>>> issues:
>>>>>>>>>>>
>>>>>>>>>>> 1) Flags api incompatibility whenever the api of an overridable
>>>>>>>>>>> api method evolves.
>>>>>>>>>>> 2) Adds relevant tests for issue (1).
>>>>>>>>>>> 3) Correctly handles the following case:
>>>>>>>>>>>
>>>>>>>>>>> Api_version_1:
>>>>>>>>>>>
>>>>>>>>>>> public class A {
>>>>>>>>>>> public void foo(A self) {
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> class B extends A {
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Api_version_2:
>>>>>>>>>>>
>>>>>>>>>>> class A as before.
>>>>>>>>>>>
>>>>>>>>>>> class B extends A{
>>>>>>>>>>> /* method overloading not overriding */
>>>>>>>>>>> public final void foo(B self) {
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Api checker was incorrectly marking this case as
>>>>>>>>>>> METHOD_OVERRIDING error. Instead, it should just mark it as
>>>>>>>>>>> METHOD_OVERLOADING error.
>>>>>>>>>>>
>>>>>>>>>>> 4) Adds test case for issue (3).
>>>>>>>>>>> 5) Adds test case for addition of unchecked exceptions.
>>>>>>>>>>> 6) Updates the config file so that 'ant apicheck' passes. I
>>>>>>>>>>> verified each change in the config file by manually inspecting the
>>>>>>>>>>> source
>>>>>>>>>>> code.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Amit
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
