Thanks. Can I have a second Review please?
> Am 01.12.2018 um 01:20 schrieb Sergey Bylokhov <sergey.bylok...@oracle.com>: > > 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.
