On Thu, 10 Apr 2025 05:51:04 GMT, Kim Barrett <[email protected]> wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> reflection improvements
>
> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
>
>> 568: * This method should usually be called after a call to
>> WhiteBox.fullGC().
>> 569: */
>> 570: public static void waitForReferenceProcessing() {
>
> Reference::waitForReferenceProcessing returns boolean. This should too.
Agreed.
> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
>
>> 568: * This method should usually be called after a call to
>> WhiteBox.fullGC().
>> 569: */
>> 570: public static void waitForReferenceProcessing() {
>
> Reference::waitForReferenceProcessing throws InterruptedException. Probably
> this should be similarly
> declared.
I disagree. IMO, for a test library, it's an unnecessary burden to make callers
catch a checked exception.
> test/lib/jdk/test/whitebox/WhiteBox.java line 570:
>
>> 568: * This method should usually be called after a call to
>> WhiteBox.fullGC().
>> 569: */
>> 570: public static void waitForReferenceProcessing() {
>
> WhiteBox functions are nearly all ordinary member functions, not static.
I saw that, but `Reference.waitForReferenceProcessing()` is itself static, and
doesn't need any context from `WhiteBox`, so I made it static.
But if `waitForReferenceProcessing()` is only expected to be called following
`fullGC()`, a WB instance will be needed anyway.
> test/lib/jdk/test/whitebox/WhiteBox.java line 574:
>
>> 572: Method wfrp =
>> Reference.class.getDeclaredMethod("waitForReferenceProcessing");
>> 573: wfrp.setAccessible(true);
>> 574: wfrp.invoke(null, new Object[0]);
>
> Don't need an empty argument vector. Sufficient is `wfrp.invoke(null);`. I
> found the documentation
> a bit confusing, as it says "the supplied args array may be ... or null."
Thanks. (I thought I tried that, but I guess not...)
> test/lib/jdk/test/whitebox/WhiteBox.java line 577:
>
>> 575: } catch (IllegalAccessException iae) {
>> 576: throw new RuntimeException("Need to add @modules
>> java.base/java.lang.ref:open?",
>> 577: iae);
>
> This isn't a useful message, as we won't get here in this case. Instead of
> `invoke` failing with this exception,
> the earlier `setAccessible` will have failed with
> `InaccessibleObjectException`.
Thanks. The new version will include the message in the case that the @modules
tag seems to be missing.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038425087
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038428891
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038432164
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038425699
PR Review Comment: https://git.openjdk.org/jdk/pull/24527#discussion_r2038424768
