Hi Chris,
Webrev updated in place:
http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/
On 2/15/2016 6:22 AM, Chris Hegarty wrote:
On 12/02/16 17:47, 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/
Overall looks ok, and satisfies the requirement of JEP 260.
It took me a while to satisfy myself that it is ok to "ignore"
the passed Signal in the wrapper's 'handle' methods. The assumption
is that this wrapper's signal field and the passes signal are, MUST,
represent the same underlying signal. Maybe an assert to make this
explicit?
The jdk.internal.misc.Signal passed to the wrapper's methods needs to be
mapped to the corresponding sun.misc.Signal but instead of maintaining a map
the s.m.Signal instance is kept in the wrapper of the s.m.SignalHandler.
Looking at j.i.m.Signal.<init>, I can see that you explicitly check
for the 'SIG' prefix and prepend it if not there, but toString() also
prepends it. Will getName also be impacted by this ( it may now have
the name prepended with 'SIG', where it previously was not ).
Yes, there was a bug there; reverting to backward compatible behavior.
HotSpot recognizes different forms of signal names on Windows and
Posix. [1]
To be compatible with previous versions, the "SIG" prefix allowed by the
HotSpot change
(only on Posix) is not supported and throws IllegalArgumentException.
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8149748
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-9149750/
Looks fine.
-Chris.
