On Fri, Dec 23, 2016 at 2:57 PM, Chris Hegarty <chris.hega...@oracle.com> wrote: > Arno, > >> On 22 Dec 2016, at 16:44, Zeller, Arno <arno.zel...@sap.com> wrote: >> >> Hi Vyom, >> >> thanks for the comments – now I understand the problem. I reworked all three >> platforms to check for exceptions and NULL if needed. >> Regarding the JNIReleaseString calls: I seem to be on the save sidether. >> They are listed as some of the few calls that can be called with a pending >> exception as described in >> http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#exception_handling >> >> Please see my new webrev: >> http://cr.openjdk.java.net/~clanger/webrevs/8170868.2/ > > I welcome this change. It is long over due, so thanks for running with it. > > I have yet to build and test the patch myself, but just a few initial > comments: > > 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. > > 2) I would prefer the use of List.of(...), and list.of() for empty, since > these are immutable and efficient list implementations. > > 3) Is the comment “Inspired ...” necessary / appropriate? >
I don't think it is necessary but in my opinion it is useful. I for myself don't know much about OS X programming and a link to some documentation seems helpful. Is it appropriate? I'm not a lawyer :) but the Disclaimer in the referenced material mentions that "Neither the name .. or logos of Apple Inc. may be used to endorse or promote products derived from the ... Software". So maybe I'd rephrase it to: /** * For more information on how to use the APIs in "CFProxySupport.h" see: * https://developer.apple.com/legacy/library/samplecode/CFProxySupportTool/Introduction/Intro.html */ > 4) Can some of the native initialization code be moved to a platform > independent location, to remove duplication? > > 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? We've discussed that before and as Christoph already mentioned this pattern is also used in other places. The rule of thumb is the following: - if we create a new file file from scratch it only has a SAP copyright - if we copy a file from an existing file and just add some fixes/minor stuff it keeps its original copyright - if we derive a new file from an existing one but add a significant amount of new code we keep the original copyright line but also add a new one from SAP There's always room for interpretation when it comes to this classification, but I think overall it is reasonable (and you can find many examples in the code base if you grep for files with more than one copyright line). > > -Chris.
