<AWT Dev> FW: RFR: [XS] 8214380: AwtDragSource function LoadCache misses a ReleaseLongArrayElements in special case

[Thomas Stüfe]

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.stuefe at sap.com> wrote:
>
>
>
> -----Original Message-----
> From: Baesken, Matthias
> Sent: Dienstag, 4. Dezember 2018 09:01
> To: Langer, Christoph <christoph.langer at sap.com>; Stuefe, Thomas <thomas.stuefe at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at 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.Bylokhov at oracle.com>
> Sent: Samstag, 1. Dezember 2018 01:18
> To: Baesken, Matthias <matthias.baesken at sap.com>; awt-dev at 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.Bylokhov at oracle.com>; 'awt-
> >> dev at openjdk.java.net' <awt-dev at 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.Bylokhov at oracle.com>; awt-
> >>> dev at 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.Bylokhov at oracle.com>
> >>>> Sent: Donnerstag, 29. November 2018 00:22
> >>>> To: Baesken, Matthias <matthias.baesken at sap.com>; awt-
> >>>> dev at 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.
>