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