On Sun, 7 May 2023 21:41:23 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Afshin Zafari has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains four commits:
>>
>> - Merge branch 'master' into _8305083
>> - 8305083: 8305083: Remove finalize() from
>> test/hotspot/jtreg/vmTestbase/nsk/share/ and /jpda that are used in
>> serviceability/dcmd/framework tests
>> - 8305083: Remove finalize() from test/hotspot/jtreg/vmTestbase/nsk/share/
>> and /jpda that are used in serviceability/dcmd/framework tests
>> - 8305083: Remove finalize() from test/hotspot/jtreg/vmTestbase/nsk/share/
>> and /jpda that are used in serviceability/dcmd/framework tests
>
> test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 30:
>
>> 28: * Finalizable interface allows <tt>Finalizer</tt> to perform
>> finalization of an object.
>> 29: * Each object that requires finalization at VM shutdown time should
>> implement this
>> 30: * interface and call the <tt>registerClenup</tt> to activate a
>> <tt>Finalizer</tt> hook.
>
> Typo: Clenup
Corrected.
> test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 61:
>
>> 59: */
>> 60: default public void registerCleanup() {
>> 61: // install finalizer to print errors summary at exit
>
> This was moved from Log but isn't appropriate for the interface. No need to
> say anything here.
Done.
> test/hotspot/jtreg/vmTestbase/nsk/share/Finalizable.java line 65:
>
>> 63: finalizer.activate();
>> 64:
>> 65: // register the cleanup method to be called when this Log
>> instance becomes unreachable.
>
> Remove "Log" - actually again this is not needed. You can see what the method
> does and it is already documented in the doc comment.
Done.
> test/hotspot/jtreg/vmTestbase/nsk/share/FinalizableObject.java line 37:
>
>> 35: /**
>> 36: * All instances of this class, should implement their own cleanup
>> method
>> 37: * to clean appropriately the objects they used.
>
> "instances" don't implement methods. Suggestion:
>
> "Subclasses should override this method to provide the specific cleanup
> actions that they need."
Suggested text is replaced.
> test/hotspot/jtreg/vmTestbase/nsk/share/MainWrapper.java line 50:
>
>> 48: finalizableObject.registerCleanup();
>> 49:
>> 50:
>
> Extra blank line not needed
Done.
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java line 410:
>
>> 408: *
>> 409: * This is replacement of the deprecated finalize() and is called
>> 410: * when this instance becomes unreachable.
>
> I don't think this is needed.
Done.
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java line 555:
>
>> 553: *
>> 554: * This is replacement of the finalize() method and is called when
>> this
>> 555: * instance becomes unreachable.
>
> Again not needed.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188303150
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188304787
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188305029
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188305875
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188306333
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188307967
PR Review Comment: https://git.openjdk.org/jdk/pull/13420#discussion_r1188308771
