RFR: 8165827: Support private interface methods in JNI, JDWP, JDI and JDB

[Daniel D. Daugherty]

On 10/11/16 3:11 PM, David Holmes wrote:
> Hi Dan,
>
> Thanks for looking at this.
>
> On 12/10/2016 3:30 AM, Daniel D. Daugherty wrote:
>> On 10/10/16 7:55 PM, David Holmes wrote:
>>> Turns out the only place changes were needed were in JDI.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8165827
>>>
>>> webrev: http://cr.openjdk.java.net/~dholmes/8165827/webrev/
>>
>> src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
>>     No comments. (Thanks for also fixing the typo.)
>>
>> src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
>>     L352:         if (isNonVirtual(options)) {
>>     L353:             if (method.isAbstract()) {
>>     L354:                 throw new IllegalArgumentException("Abstract
>> method");
>>     L355:             }
>>         Any particular reason for breaking the logic into two
>>         distinct if-statements?
>>
>>         Perhaps:
>>
>>                   if (isNonVirtual(options) && method.isAbstract()) {
>>                       throw new IllegalArgumentException("Abstract
>> method");
>>                   }
>>
>>         Also, perhaps "unexpected Abstract method" is more clear?
>
> :) I tried to forestall this comment by saying "use the same format as 
> already present in the class version of the check method. " The code I 
> have now is a copy of what is prior:
>
>  312         /*
>  313          * For nonvirtual invokes, method must have a body
>  314          */
>  315         if (isNonVirtual(options)) {
>  316             if (method.isAbstract()) {
>  317                 throw new IllegalArgumentException("Abstract 
> method");
>  318             }
>  319         }
>
> While I personally prefer the conjunctive form I went for consistency 
> whilst minimizing changes.

I'm okay with your choice.

Dan

>
>> test/com/sun/jdi/InterfaceMethodsTest.java
>>     L526:             if (t.getClass() != expectedException) {
>>     L527:                 System.err.println("--- FAILED");
>>     L528:                 failure("FAILED: " + t);
>>     L529:                 return null;
>>     L530:             }
>>         You should also report the expectedException value here to
>>         aid in failure analysis.
>
> Good point - will update.
>
>> Thumbs up! I don't need to see another webrev if you decide to
>> make the above small tweaks.
>
> Great - thanks again.
>
> David
>
>> Dan
>>
>>
>>>
>>> The spec change in ObjectReference is very simple and there is a CCC
>>> request in progress to ratify that change.
>>>
>>> The implementation change in ObjectReferenceImpl mirrors the updated
>>> spec and use the same format as already present in the class version
>>> of the check method.
>>>
>>> The test is a little more complex. This is obviously an extension to
>>> what is already tested in InterfaceMethodsTest. However IMT has a
>>> number of problem with the way it is currently written [1] -
>>> specifically it doesn't properly separate method lookup from method
>>> invocation. So I've added the capability to separate lookup and
>>> invocation for use with the private interface methods - I have not
>>> tried to address shortcomings of the existing tests. Though I did fix
>>> the return value checking logic! And did some clarifying comments and
>>> renaming in a couple of place.
>>>
>>> Still on the test I can't add the negative tests I would like to add
>>> because they actually pass due to a different long standing bug in JDI
>>> - [2]. So the actual private interface method testing is very simple:
>>> can I get the Method from the InterfaceType for the interface
>>> declaring the method? Can I then invoke that method on an instance of
>>> a class that implements the interface.
>>>
>>> Thanks,
>>> David
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8166453
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8167416
>>