Hi Chris, this change is fine for me now and I think Arno addressed all of your concerns. Is it OK for you if I push this to 9 and add you as one of the reviewers?
Thank you and best regards, Volker On Wed, Jan 11, 2017 at 3:21 PM, Zeller, Arno <arno.zel...@sap.com> wrote: > Hi Chris, > > I have addressed your comments in a new webrev. Can you please have a look at > this one? > > http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/ > > I created src/java.base/share/native/libnet/proxy_util.c/h and these files > contain now the common used native parts. > And I rewrote the code to return an array of Proxy objects from native code - > so I do no longer call back to Java to > add an object to the list. > > Best regards, > Arno > >>-----Original Message----- >>From: Chris Hegarty [mailto:chris.hega...@oracle.com] >>Sent: Dienstag, 3. Januar 2017 15:04 >>To: Zeller, Arno <arno.zel...@sap.com> >>Cc: net-dev@openjdk.java.net >>Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults >>on Windows, MacOS and Gnome >> >>Arno, >> >>> On 27 Dec 2016, at 11:44, Zeller, Arno <arno.zel...@sap.com> wrote: >>> >>> Hi Chris, >>> >>> Thanks for having a look at my change: >>> >>>> 1) It seems awful to have to deal with LinkedList in native code. How >>>> about returning an array from native, and then converting that into >>>> whatever list type is appropriate at the Java level. >>> With the current implementation on Mac I do not know upfront how many >>items the list will contain and therefore I preferred to use the LinkedList >>from >>native code. >>> I can of course change the implementation on Mac to first generate a fully >>resolved native list and then I can use NewObjectArray to generate an Array >>of Proxy objects to return. This will avoid calling to java to add an element >>to >>the list. Do you think this will be better? >> >>Yes. Thanks. >> >>>> 2) I would prefer the use of List.of(...), and list.of() for empty, since >>>> these are immutable and efficient list implementations. >>> I thought about that but I tried to not return an immutable List because the >>Javadoc of Proxy.select does not state anything about the returned List (if >>it is >>immutable or not) and I feared to break compatibility by returning an >>immutable List. >>> If you think this is no problem I will of course prefer to always return the >>same immutable NO_PROXY list object. >> >>I would prefer an immutable list. Maybe we file, a separate, issue to amend >>the spec to say "returns an immutable list …”. >> >>>> 3) Is the comment “Inspired ...” necessary / appropriate? >>> I will change it to the comment proposed by Volker. >> >>Ok. >> >>>> 4) Can some of the native initialization code be moved to a platform >>>> independent location, to remove duplication? >>> Would it be ok if I move the definition of the static variables and the >>implementation of 'static int initJavaClass(...)' to a header file in the >>shared >>branch. I.e. src/java.base/shared/native/libnet/DefaultProxySelector.h and >>include this in the MacOSX, Windows and the Unix implementations? >> >>Yes. >> >>>> 5) The new file has a shared copyright header. I see similar SAP >>>> headers in a few files, but none shared with the Oracle header. >>>> How did you arrive at this format? >>> The format was suggested to me by Volker :-) >> >>Ok. >> >>-Chris.
