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
-~----------~----~----~----~------~----~------~--~---
