Looks good to me. Thanks Yumin
On Wed, Dec 19, 2018 at 7:42 AM JC Beyler <jcbey...@google.com> wrote: > Hi all, > > Could I get a second review for this please? :) > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.01/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8215495 > > Thanks! > Jc > > On Mon, Dec 17, 2018 at 4:25 PM David Holmes <david.hol...@oracle.com> > wrote: > > > LGTM. > > > > Thanks, > > David > > > > On 18/12/2018 8:53 am, JC Beyler wrote: > > > Sounds good to me: > > > > > > For the odd corner case: > > > > > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.01/ > > > <http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.01> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8215495 > > > > > > Thanks! > > > Jc > > > > > > On Mon, Dec 17, 2018 at 2:39 PM David Holmes <david.hol...@oracle.com > > > <mailto:david.hol...@oracle.com>> wrote: > > > > > > Hi Jc, > > > > > > On 18/12/2018 8:12 am, JC Beyler wrote: > > > > Hi David, > > > > > > > > I understand the rationale behind the "If the method does return > > > NULL, > > > > then isCopy's value is undefined". But what about > > > > the DEFINE_GETSCALARARRAYELEMENTS case? > > > > > > > > It does this: > > > > if (len == 0) { \ > > > > /* Empty array: legal but useless, can't return NULL. \ > > > > * Return a pointer to something useless. \ > > > > * Avoid asserts in typeArrayOop. */ \ > > > > result = (ElementType*)get_bad_address(); \ > > > > > > > > Should we at least then put isCopy to JNI_FALSE in that case > > > since it > > > > does not return NULL and no exception is raised, > > > > > > Sure - it's an odd corner case, but better not to leave isCopy > > > potentially uninitialized. > > > > > > Thanks, > > > David > > > > > > > Thanks, > > > > Jc > > > > > > > > > > > > On Mon, Dec 17, 2018 at 1:29 PM David Holmes > > > <david.hol...@oracle.com <mailto:david.hol...@oracle.com> > > > > <mailto:david.hol...@oracle.com > > > <mailto:david.hol...@oracle.com>>> wrote: > > > > > > > > Hi Jc, > > > > > > > > On 18/12/2018 3:42 am, JC Beyler wrote: > > > > > Hi all, > > > > > > > > > > Could I get a review for this webrev: > > > > > > > > > > Webrev: > > > http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.00/ > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8215495 > > > > > > > > isCopy only has to be set if the method executes > successfully > > > > > > > > "If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a > > > copy is > > > > made; or it is set to JNI_FALSE if no copy is made." > > > > > > > > You can only make (or not) a copy if the operation actually > > > > succeeds. So > > > > before checking isCopy the caller must check for NULL and/or > > > a pending > > > > exception. > > > > > > > > I see no bug here. > > > > > > > > David > > > > ----- > > > > > > > > > Thanks, > > > > > Jc > > > > > > > > > > > > > > > > -- > > > > > > > > Thanks, > > > > Jc > > > > > > > > > > > > -- > > > > > > Thanks, > > > Jc > > > > > -- > > Thanks, > Jc >
