<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi Daniil,<br>
<br>
The fix looks pretty good to me.<br>
<br>
Just minor comments.<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html">http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html</a><br>
<pre><span class="new">2519 int skipped = 0; // skip default methods from superinterface (see JDK-8216324)
</span></pre>
<p><span class="new">You can say just:Â // skip overpass methods<br>
There is probably no need to list the bug number.<br>
</span></p>
<p><span class="new"><br>
</span></p>
<pre><span class="changed">2523 // Depending on can_maintain_original_method_order capability</span>
<span class="changed">2524 // use the original method ordering indices stored in the class, so we can emit</span>
<span class="changed">2525 // jmethodIDs in the order they appeared in the class file</span>
<span class="changed">2526 // or just copy in any order</span></pre>
Could you, please re-balance the comment a little bit?<br>
Also, the comment has to be terminated with a dot.<br>
Replace: "<span class="changed">or just copy in any order" => "</span><span
class="changed">or just copy in current order".<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html">http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html</a><br>
</span>
<pre><span class="changed"> 114 default void default_method() { } // should never be seen</span></pre>
The comment above is not clear. Should never be seen in what
context?<br>
<br>
<pre><span class="changed"> 117 interface OuterInterface1 extends DefaultInterface {</span></pre>
<span class="changed">An extra space before "extends".<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html">http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html</a><br>
<br>
I like the test simplicity.<br>
Default methods are always in interfaces.<br>
I'd suggest to rename the test to something like:
DefaultMethods.java or OverpassMethods.java.<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html">http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html</a></span><br>
<pre> 44 if (options != NULL && strcmp(options, "maintain_original_method_order") == 0) {
45 printf("maintain_original_method_order: true\n");
...
54 } else {
55 printf("maintain_original_method_order: false\n");</pre>
<span class="changed">I'd suggest to remove the lines 54 and 55
and adjust the line 45:</span><br>
  printf("Enabled capability: maintain_original_method_order:
true\n");<br>
<br>
<span class="changed"></span>
<pre> 88 jobject m = env->ToReflectedMethod(klass, methods[i], (modifiers & 8) == 8);</pre>
<span class="changed">It is better to replace 8 with a symbolic
constant.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
</span><br>
On 7/20/20 16:57, Alex Menkov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:46dafc62-bbab-9287-b398-59ca8d99d03a@oracle.com">Looks
good to me :)
<br>
Thanks for handling this.
<br>
<br>
--alex
<br>
<br>
On 07/20/2020 12:00, Daniil Titov wrote:
<br>
<blockquote type="cite">Please review change [1] that fixes
GetClassMethods behavior in cases if a default method is present
in a super interface. Currently for such cases the information
GetClassMethods returns for the sub-interface or implementing
class incorrectly includes inherited not default methods.
<br>
<br>
The fix ( thanks to Alex for providing this patch) ensures that
overpass methods are filtered out in the returns. The fix also
applies a change in the existing test as David suggested in the
comments to the issue [2].
<br>
<br>
Testing: Mach5Â tier1-tier3 tests successfully passed.
<br>
<br>
[1] <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/">http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/</a>
<br>
[2] <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8216324">https://bugs.openjdk.java.net/browse/JDK-8216324</a>
<br>
<br>
Thank you,
<br>
Daniil
<br>
<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>