[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [13] RFR 8222108: Reduce minRefreshTime for updating remote printer list on Win
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2019-08-16 22:18:42
Message-ID: ca888d58-98dc-ddda-ea54-37e3d280b795 () oracle ! com
[Download RAW message or body]

+1

On 8/16/19 12:34 pm, Phil Race wrote:
> Approved.
> 
> -phil.
> 
> On 8/16/19 5:23 AM, Alexey Ivanov wrote:
> > Hi Phil, Sergey,
> > 
> > Do you have any other comments?
> > The latest webrev:
> > http://cr.openjdk.java.net/~aivanov/8222108/webrev.02/
> > 
> > Does anybody have any additional comments?
> > 
> > 
> > I looked through the code once again, and I think there should be no null values \
> > in the array returned by getPrinterNames() unless ::EnumPrinters could return \
> > null printer names. The documentation for EnumPrinters [1] and for PRINTER_INFO_4 \
> > [2] does not mention if pPrinterName can be null or not. 
> > Anyway the custom comparator does handle null values in the printer list if there \
> > are any. 
> > Regards,
> > Alexey
> > 
> > [1] https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters
> > [2] https://docs.microsoft.com/en-us/windows/win32/printdocs/printer-info-4
> > 
> > On 03/07/2019 19:46, Alexey Ivanov wrote:
> > > Hi Phil,
> > > 
> > > Thank you for your review! That's a valid point!
> > > 
> > > Please see the updated webrev:
> > > http://cr.openjdk.java.net/~aivanov/8222108/webrev.02/
> > > 
> > > I implemented a custom comparator which handles null values in the printer \
> > > list. 
> > > However, I wonder if the list of printers can ever contain a null value. The \
> > > method refreshServices() does not check if printers[p] is null. 
> > > On 03/07/2019 00:13, Philip Race wrote:
> > > > I thought we had the checks for null in doCompare there for a reason.
> > > > Arrays.sort won't be very happy with a null element.
> > > > 
> > > > You said in the first review email of the v0 webrev :
> > > > 
> > > > > Arrays.equals() accepts null parameters and null elements in the arrays and \
> > > > > always returns the correct result.
> > > > 
> > > > but that webrev didn't use Arrays.sort and that requirement seems to have \
> > > > been forgotten when adding it.
> > > > 
> > > > public class Sort {
> > > > public static void main(String[] args) {
> > > > String[] a1 = { "a", null, "a" };
> > > > java.util.Arrays.sort(a1);
> > > > }
> > > > }
> > > > 
> > > > java Sort
> > > > Exception in thread "main" java.lang.NullPointerException
> > > > at java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:320)
> > > >  at java.util.ComparableTimSort.sort(ComparableTimSort.java:188)
> > > > at java.util.Arrays.sort(Arrays.java:1246)
> > > > at Sort.main(Sort.java:4)
> > > > 
> > > > 
> > > > -phil.
> > 
> 


-- 
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]