Hi Matthias, looks fine to me as well, though I wonder whether this lformats/saveformats coding could be simplified by simply using lformats[i] in the loops to access the array elements and getting rid of saveformats. But this has nothing to do with your patch.
Cheers, Thomas On Tue, Dec 4, 2018 at 10:09 AM Stuefe, Thomas <thomas.stu...@sap.com> wrote: > > > > -----Original Message----- > From: Baesken, Matthias > Sent: Dienstag, 4. Dezember 2018 09:01 > To: Langer, Christoph <christoph.lan...@sap.com>; Stuefe, Thomas > <thomas.stu...@sap.com>; Lindenmaier, Goetz <goetz.lindenma...@sap.com> > Subject: FW: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function LoadCache > misses a ReleaseLongArrayElements in special case > > Hallo, kann jemand noch ein 2. Review beitragen ? > > Danke und Gruß, Matthias > > > -----Original Message----- > From: Sergey Bylokhov <sergey.bylok...@oracle.com> > Sent: Samstag, 1. Dezember 2018 01:18 > To: Baesken, Matthias <matthias.baes...@sap.com>; awt-dev@openjdk.java.net > Subject: Re: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function LoadCache > misses a ReleaseLongArrayElements in special case > > Looks fine. > > On 30/11/2018 05:22, Baesken, Matthias wrote: > > Hi Sergey, I prepared a second webrev : > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8214380.1/ > > > > - removed the unused isCopy > > - added a NULL check after GetLongArrayElements > > > > > > Best regards, Matthias > > > > > > > >> -----Original Message----- > >> From: Baesken, Matthias > >> Sent: Donnerstag, 29. November 2018 10:01 > >> To: 'Sergey Bylokhov' <sergey.bylok...@oracle.com>; 'awt- > >> d...@openjdk.java.net' <awt-dev@openjdk.java.net> > >> Subject: RE: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function > >> LoadCache misses a ReleaseLongArrayElements in special case > >> > >> Btw when looking at the coding, I wonder why we save the copy-state in > >> isCopy : > >> > >> Docu : "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" > >> > >> 397 jboolean isCopy; > >> 398 jlong *lFormats = env->GetLongArrayElements(formats, &isCopy), > >> > >> But then we do not use it afterwards , so should we better call it > >> with > >> NULL instead? > >> > >> Regards, Matthias > >> > >> > >>> -----Original Message----- > >>> From: Baesken, Matthias > >>> Sent: Donnerstag, 29. November 2018 09:17 > >>> To: 'Sergey Bylokhov' <sergey.bylok...@oracle.com>; awt- > >>> d...@openjdk.java.net > >>> Subject: RE: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function > >>> LoadCache misses a ReleaseLongArrayElements in special case > >>> > >>> Hi Sergey, I think it is (at least) a good practice to NULL - check > >>> the result > >>> of env->GetLongArrayElements . > >>> Probably it should be done right after the > >>> > >>> jlong *lFormats = env->GetLongArrayElements(formats, &isCopy), > >>> > >>> call. > >>> See jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c for > >>> examples of usages of the function (with NULL checks) . > >>> > >>> Or see the documentation at > >>> > >> https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/function > >>> s.html > >>> Where we find : > >>> > >>> > >>> Get<PrimitiveType>ArrayElements Routines > >>> .... > >>> PARAMETERS: > >>> > >>> env: the JNI interface pointer. > >>> array: a Java string object. > >>> isCopy: a pointer to a boolean. > >>> > >>> > >>> RETURNS: > >>> > >>> Returns a pointer to the array elements, ****or NULL if the operation > >>> fails.**** > >>> > >>> > >>> So returning NULL might happen . > >>> > >>> Best regards, Matthias > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Sergey Bylokhov <sergey.bylok...@oracle.com> > >>>> Sent: Donnerstag, 29. November 2018 00:22 > >>>> To: Baesken, Matthias <matthias.baes...@sap.com>; awt- > >>>> d...@openjdk.java.net > >>>> Subject: Re: <AWT Dev> RFR: [XS] 8214380: AwtDragSource function > >>>> LoadCache misses a ReleaseLongArrayElements in special case > >>>> > >>>> Hi, Matthias. > >>>> > >>>> Do we need the null check in the fix, if "yes" then probably > >>>> the same check should be added before this line as well? > >>>> 465 env->ReleaseLongArrayElements(formats, saveFormats, 0); > >>>> > >>>> On 28/11/2018 00:33, Baesken, Matthias wrote: > >>>>> Hello, please review this small fix. > >>>>> > >>>>> It adds a missing ReleaseLongArrayElements to > >>>>> > >>>>> void AwtDragSource::LoadCache(jlongArray formats) > >>>>> > >>>>> in an early special "pseudo return" (leave function via throw) case. > >>>>> > >>>>> Webrev/bug : > >>>>> > >>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8214380.0/ > >>>>> > >>>>> https://bugs.openjdk.java.net/browse/JDK-8214380 > >>>>> > >>>>> Thanks, Matthias > >>>>> > >>>> > >>>> > >>>> -- > >>>> Best regards, Sergey. > > > -- > Best regards, Sergey. >
