On 16 Feb 2016, at 21:38, Roger Riggs <roger.ri...@oracle.com> wrote:
> Hi Chris, > > Webrev updated in place: > http://cr.openjdk.java.net/~rriggs/webrev-signal-9149750/ Thanks Roger, I’m happy with the source changes. -Chris. > 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. >