Hi David,
Webrev updated in place:
http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/
On 2/14/2016 11:55 PM, David Holmes wrote:
Hi Roger,
A few mostly minor comments ...
On 13/02/2016 3:47 AM, Roger Riggs wrote:
Please review moving the functionality of sun.misc.Signal to
jdk.internal.misc and
creating a wrapper sun.misc.Signal for existing external uses.
+++ This time including the hotspot changes to update the target of the
upcalls.
A new replacement API will be considered separately.
The update includes a test that passes with or without the changes.
(Except for an NPE instead of SEGV if null is passed).
Webrev:
jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/
I take it we don't care about reorder files any more?
There does not seem to be any particular guidance about how/when to use
them on Solaris.
So it seemed as reasonable to remove the entries as to update them.
---
src/java.base/share/classes/sun/misc/Signal.java
The @since should not be changed to 9. You only changed the
implementation not the API. Conversely the renamed class should
arguably be @since 9 and not @since 1.2. But it is wrong to say
sun.misc.Signal is @since 9.
The new sun.misc.Signal is a wrapper around the original implementation
now in jdk.internal.misc.
I had modified the mercurial commands to rename sun.misc.Signal to
jdk.internal.misc to preserve the history
of the implementation.
I will correct sun.misc.Signal to retain the original @since and use
@since 9 for the jdk.internal.misc
The *Handler.of method name reads strangely to me - "for" would be
better but I presume that is not allowed.
The xx.of () pattern has been used recently.
---
src/java.base/share/classes/jdk/internal/misc/Signal.java
Not sure I understand the role of NativeSignalHandler any more - given
it can't actually be used ??
They are used to retain the previous handler and can be used to restore
that handler.
The ability to invoke the handler directly was removed as a simplification.
---
test/sun/misc/SunMiscSignalTest.java
Based on our email discussion this test can not be testing that the
handler actually gets invoked - as we know it does not in some cases.
I have doubts that sun.misc.Signal ever intended to support all the
signals you are testing.
There were no tests for sun.misc.Signal. I created the tests based on
the current implementations.
They can be updated to reflect current support for handle, raise, and
whether a signal handler is called
and of course -Xrs. It should allow detection of unintended changes in
behavior.
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-9149750/
Hotspot changes are fine.
Thanks, Roger
Thanks,
David
-----
Issue:
https://bugs.openjdk.java.net/browse/JDK-8149750
Thanks, Roger
JEP 260: https://bugs.openjdk.java.net/browse/JDK-8132928
