[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-09-28 20:03:28
Message-ID: CAF9BGBxV+OqyaUv2v0zKy4RGbqoDvkrUzgk5r_kS__yhDcRbjg () mail ! gmail ! com
[Download RAW message or body]
Thanks all for the reviews, I pushed the version webrev.05 and we can
refactor and tweak the ExceptionCheckingJniEnv as we add more methods and
look at more at how it is being used,
Jc
On Fri, Sep 28, 2018 at 10:30 AM Alex Menkov <alexey.menkov@oracle.com>
wrote:
> +1 for webrev.05
>
> --alex
>
> On 09/28/2018 04:25, serguei.spitsyn@oracle.com wrote:
> > Hi Jc,
> >
> > I agree it can be refactored later so I'm Okay with the current webrev.
> >
> > Thanks,
> > Serguei
> >
> > On 9/27/18 8:57 PM, JC Beyler wrote:
> > > Hi Serguei,
> > >
> > > Exactly, I'm taking the lazy approach and just doing the ones I need.
> > > Ideally I will find a better means to wrap around the methods without
> > > having to redefine all of them but I've looked around and nothing
> > > seems really perfect even with heavy utilization of C++ templates.
> > > Perhaps I can use some macro definitions to make the code easier to be
> > > generated but I did not want to go in either direction now, instead
> > > preferring to keep it simple and direct.
> > >
> > > If you agree, as we add more methods we can always refactor this at
> > > some point if someone (or us) finds a better solution to this but that
> > > is an internal problem to the exception checking class and won't
> > > affect the tests.
> > >
> > > Does that sound reasonable?
> > >
> > > Let me know,
> > > Jc
> > >
> > > On Thu, Sep 27, 2018 at 8:00 PM <serguei.spitsyn@oracle.com
> > > <mailto:serguei.spitsyn@oracle.com>> wrote:
> > >
> > > Hi Jc,
> > >
> > > Sorry for being late to the party.
> > > The webrev5 looks good to me.
> > > I don't think you have to try to fix the build system.
> > > Avoiding using unique_ptr is good enough.
> > >
> > > Do I understand it right that the ExceptionCheckingJniEnv class
> > > is going to enhanced with more JNI functions?
> > > I'm wonder if it can be anyhow generalized to avoid this.
> > >
> > > Thanks,
> > > Serguei
> > > **
> > >
> > > On 9/27/18 2:33 PM, JC Beyler wrote:
> > > > Hi all,
> > > >
> > > > Sorry to come back to this so late in the game, but somehow when
> > > > I synced my hg clone (or the issue was always there and it is a
> > > > flaky build), it seems that something in the build might have
> > > > changed? Basically now it seems that the build is adding flags
> > > > that makes my usage of unique_ptr fail.
> > > >
> > > > I "think" it is due to the build adding the gnu++98 standard (But
> > > > this has been there for a while it seems so most likely a
> > > > side-effect is it is being now used):
> > > >
> > > > CXXSTD_CXXFLAG="-std=gnu++98"
> > > > FLAGS_CXX_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$CXXSTD_CXXFLAG
> > > > -Werror],
> > > > IF_FALSE: [CXXSTD_CXXFLAG=""])
> > > >
> > > > (from:
> > > >
> https://hg.openjdk.java.net/jdk/jdk/file/dade6dd87bb4/make/autoconf/flags-cflags.m4
> )
> > > > but I'm not sure.
> > > >
> > > > When I remove that flag, my g++ goes to a more recent standard
> > > > and unique_ptr works.
> > > >
> > > > So I now have to ask you all:
> > > > 1) Should we try to fix the build system to at least have C++11
> > > > for the C++ tests, then my webrev.04 can stay as is but has to
> > > > wait for that to happen
> > > > 2) Should we push a new version that does not use unique_ptr?
> > > > That solution would look like the following webrev:
> > > > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.05/
> > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.05/>
> > > >
> > > > Sorry for the last minute rug pull,
> > > > Jc
> > > >
> > > > On Thu, Sep 27, 2018 at 11:32 AM Mikael Vidstedt
> > > > <mikael.vidstedt@oracle.com <mailto:mikael.vidstedt@oracle.com>>
> > > > wrote:
> > > >
> > > >
> > > > Very, very nice! Thanks for adding the comment and renaming
> > > > the class! Ship it!
> > > >
> > > > Cheers,
> > > > Mikael
> > > >
> > > >
> > > > > On Sep 27, 2018, at 10:45 AM, JC Beyler <jcbeyler@google.com
> > > > > <mailto:jcbeyler@google.com>> wrote:
> > > > >
> > > > > Hi Mikael and David,
> > > > >
> > > > > @David: I thought it was implicit but did not want to
> > > > > presume on this one because my goal is to start propagating
> > > > > this new class in the test base and get the checks to be
> > > > > done implicitly so I was probably being over-cautious
> > > > > @Mikael: done and done, what do you think of the comment
> > > > > here :
> > > > >
> http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html
>
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html
>
> >
> > > > >
> > > > > For all, the new webrev is here:
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/
> > > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/>
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
> > > > >
> > > > > Thanks,
> > > > > Jc
> > > > >
> > > > > On Thu, Sep 27, 2018 at 6:03 AM David Holmes
> > > > > <david.holmes@oracle.com <mailto:david.holmes@oracle.com>>
> > > > > wrote:
> > > > >
> > > > > Sorry Jc, I thought my LGTM was implicit. :)
> > > > >
> > > > > Thanks,
> > > > > David
> > > > >
> > > > > On 26/09/2018 11:52 PM, JC Beyler wrote:
> > > > > > Ping on this, anybody have time to do a review and
> > > > > give a LGTM? Or David
> > > > > > if you have time and will to provide an explicit LGTM :)
> > > > > >
> > > > > > Thanks,
> > > > > > Jc
> > > > > >
> > > > > > On Mon, Sep 24, 2018 at 9:18 AM JC Beyler
> > > > > <jcbeyler@google.com <mailto:jcbeyler@google.com>
> > > > > > <mailto:jcbeyler@google.com
> > > > > <mailto:jcbeyler@google.com>>> wrote:
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > Sounds good to me, Alex gave me one LGTM, so it
> > > > > seems I'm waiting
> > > > > > for an explicit LGTM from you or from someone else
> > > > > on this list to
> > > > > > do a review.
> > > > > >
> > > > > > Thanks again for your help,
> > > > > > Jc
> > > > > >
> > > > > > On Sun, Sep 23, 2018 at 9:22 AM David Holmes
> > > > > > <david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>
> > > > > <mailto:david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>>> wrote:
> > > > > >
> > > > > > Hi Jc,
> > > > > >
> > > > > > I don't think it is an issue for this to go
> > > > > ahead. If the static
> > > > > > analysis tool has an issue then we can either
> > > > > find out how to
> > > > > > teach it
> > > > > > about this code structure, or else flag the
> > > > > issues as false
> > > > > > positives.
> > > > > > I'd be relying on one of the serviceability
> > > > > team here to handle
> > > > > > that if
> > > > > > the problem arises.
> > > > > >
> > > > > > Thanks,
> > > > > > David
> > > > > >
> > > > > > On 23/09/2018 12:17 PM, JC Beyler wrote:
> > > > > > > Hi David,
> > > > > > >
> > > > > > > No worries with the time to answer; I
> > > > > appreciate the review!
> > > > > > >
> > > > > > > That's a fair point. Is it possible to
> > > > > "describe" to the
> > > > > > static analysis
> > > > > > > tool to "trust" calls made in the
> > > > > SafeJNIEnv class?
> > > > > > >
> > > > > > > Otherwise, do you have any idea on how to
> > > > > go forward? For
> > > > > > what it's
> > > > > > > worth, this current webrev does not try to
> > > > > replace exception
> > > > > > checking
> > > > > > > with the SafeJNIEnv (it actually adds it),
> > > > > so for now the
> > > > > > > question/solution could be delayed. I could
> > > > > continue working
> > > > > > on this in
> > > > > > > the scope of both the nsk/gc/lock/jniref
> > > > > code base and the
> > > > > > NSK_VERIFIER
> > > > > > > macro removal and we can look at how to
> > > > > tackle the cases
> > > > > > where the tests
> > > > > > > are actually calling exception checking (I
> > > > > know my
> > > > > > heapmonitor does for
> > > > > > > example).
> > > > > > >
> > > > > > > Let me know what you think and thanks for
> > > > > the review,
> > > > > > > Jc
> > > > > > >
> > > > > > >
> > > > > > > On Sun, Sep 23, 2018 at 6:48 AM David Holmes
> > > > > > <david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>
> > > > > <mailto:david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>>
> > > > > > > <mailto:david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>
> > > > > > <mailto:david.holmes@oracle.com
> > > > > <mailto:david.holmes@oracle.com>>>> wrote:
> > > > > > >
> > > > > > > Hi Jc,
> > > > > > >
> > > > > > > Sorry for the delay on getting back to
> > > > > this but I'm
> > > > > > travelling at the
> > > > > > > moment.
> > > > > > >
> > > > > > > This looks quite neat. Thanks also to
> > > > > Alex for all the
> > > > > > suggestions.
> > > > > > >
> > > > > > > My only remaining concern is that
> > > > > static analysis tools
> > > > > > may not like
> > > > > > > this because they may not be able to
> > > > > determine that we
> > > > > > won't make
> > > > > > > subsequent JNI calls when an exception
> > > > > happens in the
> > > > > > first. That's not
> > > > > > > a reason not to do this of course, just
> > > > > flagging that we
> > > > > > may have to do
> > > > > > > something to deal with that problem.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > David
> > > > > > >
> > > > > > > On 20/09/2018 11:56 AM, JC Beyler wrote:
> > > > > > > > Hi Alex,
> > > > > > > >
> > > > > > > > Done here, thanks for the review:
> > > > > > > >
> > > > > > > > Webrev:
> > > > > >
> > > > > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>
> > > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>
> > > > > > > >
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/>
> > > > > > > > Bug:
> > > > > https://bugs.openjdk.java.net/browse/JDK-8210842
> > > > > > > >
> > > > > > > > Thanks again!
> > > > > > > > Jc
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Sep 19, 2018 at 5:13 PM Alex
> > > > > Menkov
> > > > > > > <alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>
> > > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>>> wrote:
> > > > > > > >
> > > > > > > > Hi Jc,
> > > > > > > >
> > > > > > > > Looks good to me.
> > > > > > > > A minor note:
> > > > > > > > - I'd move ErrorHandler typedef
> > > > > to SafeJNIEnv
> > > > > > class to avoid
> > > > > > > global
> > > > > > > > namespece pollution
> > > > > (ErrorHandler is too generic
> > > > > > name).
> > > > > > > >
> > > > > > > > --alex
> > > > > > > >
> > > > > > > > On 09/19/2018 15:56, JC Beyler
> > > > > wrote:
> > > > > > > > > Hi Alex,
> > > > > > > > >
> > > > > > > > > I've updated the webrev to:
> > > > > > > > > Webrev:
> > > > > > >
> > > > > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > > > > > > >
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > > > > > > > >
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/>
> > > > > > > > > Bug:
> > > > > > https://bugs.openjdk.java.net/browse/JDK-8210842
> > > > > > > > >
> > > > > > > > > That webrev has the code that
> > > > > is shown here in
> > > > > > snippets.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks for the reviews, I've
> > > > > relatively
> > > > > > followed your reviews
> > > > > > > > except for
> > > > > > > > > one detail due to me wanting
> > > > > to handle the
> > > > > > NSK_JNI_VERIFY
> > > > > > > macros via
> > > > > > > > > this system as well later
> > > > > down the road. For an
> > > > > > example:
> > > > > > > > >
> > > > > > > > > We currently have in the code:
> > > > > > > > > if ( ! NSK_JNI_VERIFY(pEnv,
> > > > > (mhClass =
> > > > > > > NSK_CPP_STUB2(GetObjectClass,
> > > > > > > > > pEnv, mhToCall)) != NULL) )
> > > > > > > > >
> > > > > > > > > 1) The NSK_CPP_STUB2 is
> > > > > trivially removed with
> > > > > > a refactor
> > > > > > > > (JDK-8210728)
> > > > > > > > >
> > > > > >
> > > > > <https://bugs.openjdk.java.net/browse/JDK-8210728> to:
> > > > > > > > > if ( ! NSK_JNI_VERIFY(pEnv,
> > > > > (mhClass =
> > > > > > > pEnv->GetObjectClass(pEnv,
> > > > > > > > > mhToCall)) != NULL) )
> > > > > > > > >
> > > > > > > > > 2) Then the NSK_JNI_VERIFY,
> > > > > I'd like to remove
> > > > > > it to and it
> > > > > > > > becomes via
> > > > > > > > > this wrapping of JNIEnv:
> > > > > > > > > if ( ! (mhClass =
> > > > > pEnv->GetObjectClass(pEnv,
> > > > > > mhToCall)) )
> > > > > > > > >
> > > > > > > > > 3) Then, via removing the
> > > > > assignment, we'd
> > > > > > arrive to a:
> > > > > > > > > mhClass =
> > > > > pEnv->GetObjectClass(pEnv, mhToCall));
> > > > > > > > > if (!mhClass)
> > > > > > > > >
> > > > > > > > > Without any loss of checking
> > > > > for failures, etc.
> > > > > > > > >
> > > > > > > > > So that is my motivation for
> > > > > most of this work
> > > > > > with a concrete
> > > > > > > > example
> > > > > > > > > (hopefully it helps drive
> > > > > this conversation).
> > > > > > > > >
> > > > > > > > > I inlined my answers here,
> > > > > let me know what you
> > > > > > think.
> > > > > > > > >
> > > > > > > > > On Wed, Sep 19, 2018 at 2:30
> > > > > PM Alex Menkov
> > > > > > > > <alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>
> > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>>
> > > > > > > > >
> > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>
> > > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>
> > > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>
> > > > > > <mailto:alexey.menkov@oracle.com
> > > > > <mailto:alexey.menkov@oracle.com>>>>>> wrote:
> > > > > > > > >
> > > > > > > > > Hi Jc,
> > > > > > > > >
> > > > > > > > > Updated tests looks good.
> > > > > > > > > Some notes about
> implementation.
> > > > > > > > >
> > > > > > > > > - FatalOnException
> > > > > implements both
> > > > > > verification and error
> > > > > > > > handling
> > > > > > > > > It would be better to
> > > > > separate them (and
> > > > > > this makes
> > > > > > > easy to
> > > > > > > > implement
> > > > > > > > > error handling different from
> > > > > > JNIEnv::FatalError).
> > > > > > > > > The simplest way is to
> > > > > define error handler as
> > > > > > > > > class SafeJNIEnv {
> > > > > > > > > public:
> > > > > > > > > typedef void
> > > > > (*ErrorHandler)(JNIEnv
> > > > > > *env, const
> > > > > > > char*
> > > > > > > > errorMsg);
> > > > > > > > > // error handler which
> > > > > terminates jvm
> > > > > > by using
> > > > > > > FatalError()
> > > > > > > > > static void
> > > > > FatalError(JNIEnv *env,
> > > > > > const char
> > > > > > > *errrorMsg);
> > > > > > > > >
> > > > > > > > > SafeJNIEnv(JNIEnv*
> > > > > jni_env, ErrorHandler
> > > > > > > errorHandler =
> > > > > > > > > FatalError);
> > > > > > > > > (SafeJNIEnv methods should
> > > > > create
> > > > > > FatalOnException objects
> > > > > > > > passing
> > > > > > > > > errorHandler)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Agreed, I tried to keep the
> > > > > code simple. The
> > > > > > concepts you
> > > > > > > talk about
> > > > > > > > > here are generally what I
> > > > > reserve for when I
> > > > > > need it (ie
> > > > > > > > extensions and
> > > > > > > > > handling new cases). But a
> > > > > lot are going to be
> > > > > > needed soon
> > > > > > > so I
> > > > > > > > think it
> > > > > > > > > is a good time to iron the
> > > > > code out now on this
> > > > > > "simple"
> > > > > > > webrev.
> > > > > > > > >
> > > > > > > > > So done for this.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > - FatalOnException is used
> > > > > in SafeJNIEnv
> > > > > > methods as
> > > > > > > > > FatalOnException
> > > > > marker(this, "msg");
> > > > > > > > > ret = <JNI call>
> > > > > > > > >
> > > > > (optional)marker.check_for_null(ret);
> > > > > > > > > return ret;
> > > > > > > > > But actually I'd call it
> > > > > something like
> > > > > > > JNICallResultVerifier and
> > > > > > > > > create
> > > > > > > > > the object after JNI call.
> like
> > > > > > > > > ret = <JNI call>
> > > > > > > > >
> > > > > JNICallResultVerifier(this, "msg")
> > > > > > > > > (optional).notNull(ret);
> > > > > > > > > return ret;
> > > > > > > > > or even (if you like such
> > > > > syntax sugar) you
> > > > > > can define
> > > > > > > > > template<typename T>
> > > > > > > > > T resultNotNull(T result)
> {
> > > > > > > > > notNull(result);
> > > > > > > > > return result;
> > > > > > > > > }
> > > > > > > > > and do
> > > > > > > > > ret = <JNI call>
> > > > > > > > > return
> > > > > JNICallResultVerifier(this,
> > > > > > > "msg").resultNotNull(ret);
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > So I renamed FatalOnException
> > > > > to now being
> > > > > > JNIVerifier.
> > > > > > > > >
> > > > > > > > > Though I like it, I don't
> > > > > think we can do it,
> > > > > > except if we
> > > > > > > do it
> > > > > > > > > slightly differently:
> > > > > > > > > I'm trying to solve two
> > > > > problems with one stone:
> > > > > > > > > - How to check for returns
> > > > > of JNI calls in
> > > > > > the way that is
> > > > > > > > done here
> > > > > > > > > - How to remove
> > > > > NSK_JNI_VERIFY* (from
> > > > > > > nsk/share/jni/jni_tools)
> > > > > > > > >
> > > > > > > > > However, the NSK_JNI_VERIFY
> > > > > need to start a
> > > > > > tracing system
> > > > > > > before
> > > > > > > > the
> > > > > > > > > call to JNI, so it won't work
> > > > > this way. (Side
> > > > > > question
> > > > > > > would be
> > > > > > > > do we
> > > > > > > > > still care about the tracing
> > > > > in NSK_JNI_VERIFY,
> > > > > > if we
> > > > > > > don't then
> > > > > > > > your
> > > > > > > > > solution works well in most
> > > > > situations).
> > > > > > > > >
> > > > > > > > > My vision or intuition is
> > > > > that we would throw a
> > > > > > template
> > > > > > > at some
> > > > > > > > point
> > > > > > > > > on SafeJNIEnv to handle both
> > > > > cases and have
> > > > > > JNIVerifier
> > > > > > > become a
> > > > > > > > > specialization in certain
> > > > > cases and something
> > > > > > different
> > > > > > > for the
> > > > > > > > > NSK_JNI_VERIFY case (or have
> > > > > a different
> > > > > > constructor to enable
> > > > > > > > tracing).
> > > > > > > > > But for now, I offer the
> > > > > implementation that does:
> > > > > > > > >
> > > > > > > > > jclass
> > > > > SafeJNIEnv::GetObjectClass(jobject obj) {
> > > > > > > > > JNIVerifier<jclass>
> marker(this,
> > > > > > "GetObjectClass");
> > > > > > > > > return
> > > > > > marker.ResultNotNull(_jni_env->GetObjectClass(obj));
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > and:
> > > > > > > > >
> > > > > > > > > void
> > > > > SafeJNIEnv::SetObjectField(jobject obj,
> > > > > > jfieldID
> > > > > > > field, jobject
> > > > > > > > > value) {
> > > > > > > > > JNIVerifier<> marker(this,
> > > > > "SetObjectField");
> > > > > > > > > _jni_env->SetObjectField(obj,
> > > > > field, value);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > - you added #include
> > > > > <memory> in the test
> > > > > > (and you have to
> > > > > > > > add it to
> > > > > > > > > every test).
> > > > > > > > > It would be simpler to add
> > > > > the include to
> > > > > > > SafeJNIEnv.hpp and
> > > > > > > > define
> > > > > > > > > typedef
> > > > > std::unique_ptr<SafeJNIEnv>
> > > > > > SafeJNIEnvPtr;
> > > > > > > > > Then each in the test methods:
> > > > > > > > > SafeJNIEnvPtr env(new
> > > > > SafeJNIEnv(jni_env));
> > > > > > > > > or you can add
> > > > > > > > > static
> SafeJNIEnv::SafeJNIEnvPtr
> > > > > > wrap(JNIEnv* jni_env,
> > > > > > > > ErrorHandler
> > > > > > > > > errorHandler = fatalError)
> > > > > > > > > and get
> > > > > > > > > SafeJNIEnvPtr env =
> > > > > > SafeJNIEnv::wrap(jni_env);
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Done, I like that, even less
> > > > > code change to
> > > > > > tests then.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > - it would be better to wrap
> > > > > internal classes
> > > > > > > > (FatalOnException) into
> > > > > > > > > unnamed namespace - this
> > > > > helps to avoid
> > > > > > conflicts with
> > > > > > > other
> > > > > > > > classes)
> > > > > > > > >
> > > > > > > > > -
> > > > > FatalOnException::check_for_null(void* ptr)
> > > > > > > > > should be
> > > > > > > > >
> > > > > FatalOnException::check_for_null(const
> > > > > > void* ptr)
> > > > > > > > > And calling the method you
> > > > > don't need
> > > > > > reinterpret_cast
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Also done, it got renamed to
> > > > > ResultNotNull, is
> > > > > > using a
> > > > > > > template
> > > > > > > > now, but
> > > > > > > > > agreed, that worked.
> > > > > > > > > Thanks again for the review,
> > > > > > > > > Jc
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --alex
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 09/18/2018 11:07, JC
> > > > > Beyler wrote:
> > > > > > > > > > Hi David,
> > > > > > > > > >
> > > > > > > > > > Thanks for the quick
> > > > > review and
> > > > > > thoughts. I have
> > > > > > > now a new
> > > > > > > > > version here
> > > > > > > > > > that addresses your
> comments:
> > > > > > > > > >
> > > > > > > > > > Webrev:
> > > > > > > >
> > > > > >
> > > > > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > > > >
> > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > > > > >
> > > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > > > > > >
> > > > > > >
> > > > > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/>
> > > > > > > > > >
> > > > > >
> > > > > Bug:https://bugs.openjdk.java.net/browse/JDK-8210842
> > > > > > > > > >
> > > > > > > > > > I've also inlined my
> > > > > answers/comments.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > I've slowly
> > > > > started working on
> > > > > > JDK-8191519
> > > > > > > > > > >
> > > > > > >
> > > > > <https://bugs.openjdk.java.net/browse/JDK-8191519>.
> > > > > > > > > However before
> > > > > > > > > > > starting to really
> > > > > refactor all
> > > > > > the tests, I
> > > > > > > > thought I'd
> > > > > > > > > get a few
> > > > > > > > > > > opinions. I am
> > > > > working on
> > > > > > internalizing the
> > > > > > > error
> > > > > > > > handling
> > > > > > > > > of JNI
> > > > > > > > > > calls
> > > > > > > > > > > via a SafeJNIEnv
> > > > > class that
> > > > > > redefines all
> > > > > > > the JNI
> > > > > > >
--
Thanks,
Jc
[Attachment #3 (text/html)]
<div dir="ltr">Thanks all for the reviews, I pushed the version webrev.05 and we can \
refactor and tweak the ExceptionCheckingJniEnv as we add more methods and look at \
more at how it is being used,<div>Jc</div></div><br><div class="gmail_quote"><div \
dir="ltr">On Fri, Sep 28, 2018 at 10:30 AM Alex Menkov <<a \
href="mailto:alexey.menkov@oracle.com">alexey.menkov@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">+1 for webrev.05<br> <br>
--alex<br>
<br>
On 09/28/2018 04:25, <a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> wrote:<br> > Hi Jc,<br>
> <br>
> I agree it can be refactored later so I'm Okay with the current webrev.<br>
> <br>
> Thanks,<br>
> Serguei<br>
> <br>
> On 9/27/18 8:57 PM, JC Beyler wrote:<br>
>> Hi Serguei,<br>
>><br>
>> Exactly, I'm taking the lazy approach and just doing the ones I need. \
<br> >> Ideally I will find a better means to wrap around the methods without \
<br> >> having to redefine all of them but I've looked around and nothing \
<br> >> seems really perfect even with heavy utilization of C++ templates. <br>
>> Perhaps I can use some macro definitions to make the code easier to be <br>
>> generated but I did not want to go in either direction now, instead <br>
>> preferring to keep it simple and direct.<br>
>><br>
>> If you agree, as we add more methods we can always refactor this at <br>
>> some point if someone (or us) finds a better solution to this but that <br>
>> is an internal problem to the exception checking class and won't <br>
>> affect the tests.<br>
>><br>
>> Does that sound reasonable?<br>
>><br>
>> Let me know,<br>
>> Jc<br>
>><br>
>> On Thu, Sep 27, 2018 at 8:00 PM <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> <br> >> <mailto:<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>>> wrote:<br> >><br>
>> Hi Jc,<br>
>><br>
>> Sorry for being late to the party.<br>
>> The webrev5 looks good to me.<br>
>> I don't think you have to try to fix the build system.<br>
>> Avoiding using unique_ptr is good enough.<br>
>><br>
>> Do I understand it right that the ExceptionCheckingJniEnv class<br>
>> is going to enhanced with more JNI functions?<br>
>> I'm wonder if it can be anyhow generalized to avoid this.<br>
>><br>
>> Thanks,<br>
>> Serguei<br>
>> **<br>
>><br>
>> On 9/27/18 2:33 PM, JC Beyler wrote:<br>
>>> Hi all,<br>
>>><br>
>>> Sorry to come back to this so late in the game, but somehow when<br>
>>> I synced my hg clone (or the issue was always there and it is a<br>
>>> flaky build), it seems that something in the build might have<br>
>>> changed? Basically now it seems that the build is adding flags<br>
>>> that makes my usage of unique_ptr fail.<br>
>>><br>
>>> I "think" it is due to the build adding the gnu++98 \
standard (But<br> >>> this has been there for a while it seems so most \
likely a<br> >>> side-effect is it is being now used):<br>
>>><br>
>>> CXXSTD_CXXFLAG="-std=gnu++98"<br>
>>> FLAGS_CXX_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$CXXSTD_CXXFLAG<br>
>>> -Werror],<br>
>>> IF_FALSE: [CXXSTD_CXXFLAG=""])<br>
>>><br>
>>> (from:<br>
>>> <a href="https://hg.openjdk.java.net/jdk/jdk/file/dade6dd87bb4/make/autoconf/flags-cflags.m4" \
rel="noreferrer" target="_blank">https://hg.openjdk.java.net/jdk/jdk/file/dade6dd87bb4/make/autoconf/flags-cflags.m4</a>)<br>
>>> but I'm not sure.<br>
>>><br>
>>> When I remove that flag, my g++ goes to a more recent standard<br>
>>> and unique_ptr works.<br>
>>><br>
>>> So I now have to ask you all:<br>
>>> 1) Should we try to fix the build system to at least have \
C++11<br> >>> for the C++ tests, then my webrev.04 can stay as is but \
has to<br> >>> wait for that to happen<br>
>>> 2) Should we push a new version that does not use unique_ptr?<br>
>>> That solution would look like the following webrev:<br>
>>> <a href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.05/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.05/</a><br>
>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.05/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.05/</a>><br> \
>>><br> >>> Sorry for the last minute rug pull,<br>
>>> Jc<br>
>>><br>
>>> On Thu, Sep 27, 2018 at 11:32 AM Mikael Vidstedt<br>
>>> <<a href="mailto:mikael.vidstedt@oracle.com" \
target="_blank">mikael.vidstedt@oracle.com</a> <mailto:<a \
href="mailto:mikael.vidstedt@oracle.com" \
target="_blank">mikael.vidstedt@oracle.com</a>>><br> >>> \
wrote:<br> >>><br>
>>><br>
>>> Very, very nice! Thanks for adding the comment and renaming<br>
>>> the class! Ship it!<br>
>>><br>
>>> Cheers,<br>
>>> Mikael<br>
>>><br>
>>><br>
>>>> On Sep 27, 2018, at 10:45 AM, JC Beyler <<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a><br> \
>>>> <mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a>>> wrote:<br> >>>><br>
>>>> Hi Mikael and David,<br>
>>>><br>
>>>> @David: I thought it was implicit but did not want to<br>
>>>> presume on this one because my goal is to start \
propagating<br> >>>> this new class in the test base and get the \
checks to be<br> >>>> done implicitly so I was probably being \
over-cautious<br> >>>> @Mikael: done and done, what do you think \
of the comment<br> >>>> here :<br>
>>>> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.0 \
4/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html</a><br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev \
.04/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.html</a>><br>
>>>><br>
>>>> For all, the new webrev is here:<br>
>>>> Webrev:<br>
>>>> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.04/</a><br> \
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.04/</a>><br> \
>>>> Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8210842" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210842</a><br> \
>>>><br> >>>> Thanks,<br>
>>>> Jc<br>
>>>><br>
>>>> On Thu, Sep 27, 2018 at 6:03 AM David Holmes<br>
>>>> <<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a> <mailto:<a \
href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>>><br> >>>> \
wrote:<br> >>>><br>
>>>> Sorry Jc, I thought my LGTM was implicit. :)<br>
>>>><br>
>>>> Thanks,<br>
>>>> David<br>
>>>><br>
>>>> On 26/09/2018 11:52 PM, JC Beyler wrote:<br>
>>>> > Ping on this, anybody have time to do a review \
and<br> >>>> give a LGTM? Or David<br>
>>>> > if you have time and will to provide an explicit \
LGTM :)<br> >>>> ><br>
>>>> > Thanks,<br>
>>>> > Jc<br>
>>>> ><br>
>>>> > On Mon, Sep 24, 2018 at 9:18 AM JC Beyler<br>
>>>> <<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a> <mailto:<a \
href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>><br> \
>>>> > <mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a><br> >>>> \
<mailto:<a href="mailto:jcbeyler@google.com" \
target="_blank">jcbeyler@google.com</a>>>> wrote:<br> >>>> \
><br> >>>> > Hi David,<br>
>>>> ><br>
>>>> > Sounds good to me, Alex gave me one LGTM, so \
it<br> >>>> seems I'm waiting<br>
>>>> > for an explicit LGTM from you or from someone \
else<br> >>>> on this list to<br>
>>>> > do a review.<br>
>>>> ><br>
>>>> > Thanks again for your help,<br>
>>>> > Jc<br>
>>>> ><br>
>>>> > On Sun, Sep 23, 2018 at 9:22 AM David \
Holmes<br> >>>> > <<a \
href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a><br> \
>>>> <mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>>>> wrote:<br> >>>> \
><br> >>>> > Hi Jc,<br>
>>>> ><br>
>>>> > I don't think it is an issue for this \
to go<br> >>>> ahead. If the static<br>
>>>> > analysis tool has an issue then we can \
either<br> >>>> find out how to<br>
>>>> > teach it<br>
>>>> > about this code structure, or else flag \
the<br> >>>> issues as false<br>
>>>> > positives.<br>
>>>> > I'd be relying on one of the \
serviceability<br> >>>> team here to handle<br>
>>>> > that if<br>
>>>> > the problem arises.<br>
>>>> ><br>
>>>> > Thanks,<br>
>>>> > David<br>
>>>> ><br>
>>>> > On 23/09/2018 12:17 PM, JC Beyler \
wrote:<br> >>>> > > Hi David,<br>
>>>> > ><br>
>>>> > > No worries with the time to answer; \
I<br> >>>> appreciate the review!<br>
>>>> > ><br>
>>>> > > That's a fair point. Is it \
possible to<br> >>>> "describe" to the<br>
>>>> > static analysis<br>
>>>> > > tool to "trust" calls made \
in the<br> >>>> SafeJNIEnv class?<br>
>>>> > ><br>
>>>> > > Otherwise, do you have any idea on \
how to<br> >>>> go forward? For<br>
>>>> > what it's<br>
>>>> > > worth, this current webrev does not \
try to<br> >>>> replace exception<br>
>>>> > checking<br>
>>>> > > with the SafeJNIEnv (it actually adds \
it),<br> >>>> so for now the<br>
>>>> > > question/solution could be delayed. I \
could<br> >>>> continue working<br>
>>>> > on this in<br>
>>>> > > the scope of both the \
nsk/gc/lock/jniref<br> >>>> code base and the<br>
>>>> > NSK_VERIFIER<br>
>>>> > > macro removal and we can look at how \
to<br> >>>> tackle the cases<br>
>>>> > where the tests<br>
>>>> > > are actually calling exception \
checking (I<br> >>>> know my<br>
>>>> > heapmonitor does for<br>
>>>> > > example).<br>
>>>> > ><br>
>>>> > > Let me know what you think and thanks \
for<br> >>>> the review,<br>
>>>> > > Jc<br>
>>>> > ><br>
>>>> > ><br>
>>>> > > On Sun, Sep 23, 2018 at 6:48 AM David \
Holmes<br> >>>> > <<a \
href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a><br> \
>>>> <mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>>><br> >>>> \
> > <mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>><br> >>>> > \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:david.holmes@oracle.com" \
target="_blank">david.holmes@oracle.com</a>>>>> wrote:<br> \
>>>> > ><br> >>>> > \
> Hi Jc,<br> >>>> > ><br>
>>>> > > Sorry for the delay on getting \
back to<br> >>>> this but I'm<br>
>>>> > travelling at the<br>
>>>> > > moment.<br>
>>>> > ><br>
>>>> > > This looks quite neat. Thanks \
also to<br> >>>> Alex for all the<br>
>>>> > suggestions.<br>
>>>> > ><br>
>>>> > > My only remaining concern is \
that<br> >>>> static analysis tools<br>
>>>> > may not like<br>
>>>> > > this because they may not be able \
to<br> >>>> determine that we<br>
>>>> > won't make<br>
>>>> > > subsequent JNI calls when an \
exception<br> >>>> happens in the<br>
>>>> > first. That's not<br>
>>>> > > a reason not to do this of \
course, just<br> >>>> flagging that we<br>
>>>> > may have to do<br>
>>>> > > something to deal with that \
problem.<br> >>>> > ><br>
>>>> > > Thanks,<br>
>>>> > > David<br>
>>>> > ><br>
>>>> > > On 20/09/2018 11:56 AM, JC Beyler \
wrote:<br> >>>> > > > Hi Alex,<br>
>>>> > > ><br>
>>>> > > > Done here, thanks for the \
review:<br> >>>> > > ><br>
>>>> > > > Webrev:<br>
>>>> ><br>
>>>> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/</a><br> \
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/</a>><br> \
>>>> > <br> >>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/</a>><br> \
>>>> > > <br> >>>> \
<<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/</a>><br>
>>>> > > ><br>
>>>> > <br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.03/</a>><br> \
>>>> > > > Bug:<br> >>>> \
<a href="https://bugs.openjdk.java.net/browse/JDK-8210842" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210842</a><br> \
>>>> > > ><br> >>>> \
> > > Thanks again!<br> >>>> > \
> > Jc<br> >>>> > > ><br>
>>>> > > ><br>
>>>> > > > On Wed, Sep 19, 2018 at \
5:13 PM Alex<br> >>>> Menkov<br>
>>>> > > <<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>><br> >>>> \
> > > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>>>> wrote:<br> \
>>>> > > ><br> >>>> \
> > > Hi Jc,<br> >>>> > \
> ><br> >>>> > > > \
Looks good to me.<br> >>>> > > > A \
minor note:<br> >>>> > > > - \
I'd move ErrorHandler typedef<br> >>>> to SafeJNIEnv<br>
>>>> > class to avoid<br>
>>>> > > global<br>
>>>> > > > namespece pollution<br>
>>>> (ErrorHandler is too generic<br>
>>>> > name).<br>
>>>> > > ><br>
>>>> > > > --alex<br>
>>>> > > ><br>
>>>> > > > On 09/19/2018 15:56, JC \
Beyler<br> >>>> wrote:<br>
>>>> > > > > Hi Alex,<br>
>>>> > > > ><br>
>>>> > > > > I've updated \
the webrev to:<br> >>>> > > > \
> Webrev:<br> >>>> > ><br>
>>>> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/</a><br> \
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/</a>><br> \
>>>> > <br> >>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/</a>><br> \
>>>> > > <br> >>>> \
<<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/</a>><br>
>>>> > > ><br>
>>>> > <br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/</a>><br> \
>>>> > > > ><br> \
>>>> > <br> >>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.02/</a>><br> \
>>>> > > > > Bug:<br> \
>>>> > <a \
href="https://bugs.openjdk.java.net/browse/JDK-8210842" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210842</a><br> \
>>>> > > > ><br> \
>>>> > > > > That webrev has \
the code that<br> >>>> is shown here in<br>
>>>> > snippets.<br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > Thanks for the \
reviews, I've<br> >>>> relatively<br>
>>>> > followed your reviews<br>
>>>> > > > except for<br>
>>>> > > > > one detail due to \
me wanting<br> >>>> to handle the<br>
>>>> > NSK_JNI_VERIFY<br>
>>>> > > macros via<br>
>>>> > > > > this system as \
well later<br> >>>> down the road. For an<br>
>>>> > example:<br>
>>>> > > > ><br>
>>>> > > > > We currently have \
in the code:<br> >>>> > > > > \
if ( ! NSK_JNI_VERIFY(pEnv,<br> >>>> (mhClass =<br>
>>>> > > NSK_CPP_STUB2(GetObjectClass,<br>
>>>> > > > > pEnv, mhToCall)) \
!= NULL) )<br> >>>> > > > \
><br> >>>> > > > > 1) The \
NSK_CPP_STUB2 is<br> >>>> trivially removed with<br>
>>>> > a refactor<br>
>>>> > > > (JDK-8210728)<br>
>>>> > > > ><br>
>>>> > <br>
>>>> <<a \
href="https://bugs.openjdk.java.net/browse/JDK-8210728" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210728</a>> to:<br> \
>>>> > > > > if ( ! \
NSK_JNI_VERIFY(pEnv,<br> >>>> (mhClass =<br>
>>>> > > pEnv->GetObjectClass(pEnv,<br>
>>>> > > > > mhToCall)) != \
NULL) )<br> >>>> > > > ><br>
>>>> > > > > 2) Then the \
NSK_JNI_VERIFY,<br> >>>> I'd like to remove<br>
>>>> > it to and it<br>
>>>> > > > becomes via<br>
>>>> > > > > this wrapping of \
JNIEnv:<br> >>>> > > > > if ( \
! (mhClass =<br> >>>> pEnv->GetObjectClass(pEnv,<br>
>>>> > mhToCall)) )<br>
>>>> > > > ><br>
>>>> > > > > 3) Then, via \
removing the<br> >>>> assignment, we'd<br>
>>>> > arrive to a:<br>
>>>> > > > > mhClass =<br>
>>>> pEnv->GetObjectClass(pEnv, mhToCall));<br>
>>>> > > > > if (!mhClass)<br>
>>>> > > > ><br>
>>>> > > > > Without any loss \
of checking<br> >>>> for failures, etc.<br>
>>>> > > > ><br>
>>>> > > > > So that is my \
motivation for<br> >>>> most of this work<br>
>>>> > with a concrete<br>
>>>> > > > example<br>
>>>> > > > > (hopefully it \
helps drive<br> >>>> this conversation).<br>
>>>> > > > ><br>
>>>> > > > > I inlined my \
answers here,<br> >>>> let me know what you<br>
>>>> > think.<br>
>>>> > > > ><br>
>>>> > > > > On Wed, Sep 19, \
2018 at 2:30<br> >>>> PM Alex Menkov<br>
>>>> > > > <<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>><br> >>>> \
> > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>>><br> >>>> \
> > > ><br> >>>> <mailto:<a \
href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>><br> >>>> \
> > > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>><br> >>>> \
> > <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>><br> >>>> \
> <mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a><br> >>>> \
<mailto:<a href="mailto:alexey.menkov@oracle.com" \
target="_blank">alexey.menkov@oracle.com</a>>>>>>> wrote:<br> \
>>>> > > > ><br> \
>>>> > > > > Hi Jc,<br> \
>>>> > > > ><br> \
>>>> > > > > Updated tests \
looks good.<br> >>>> > > > > \
Some notes about implementation.<br> >>>> > > \
> ><br> >>>> > > > \
> - FatalOnException<br> >>>> implements both<br>
>>>> > verification and error<br>
>>>> > > > handling<br>
>>>> > > > > It would be \
better to<br> >>>> separate them (and<br>
>>>> > this makes<br>
>>>> > > easy to<br>
>>>> > > > implement<br>
>>>> > > > > error handling \
different from<br> >>>> > JNIEnv::FatalError).<br>
>>>> > > > > The simplest way \
is to<br> >>>> define error handler as<br>
>>>> > > > > class SafeJNIEnv \
{<br> >>>> > > > > \
public:<br> >>>> > > > > \
typedef void<br> >>>> (*ErrorHandler)(JNIEnv<br>
>>>> > *env, const<br>
>>>> > > char*<br>
>>>> > > > errorMsg);<br>
>>>> > > > > // error \
handler which<br> >>>> terminates jvm<br>
>>>> > by using<br>
>>>> > > FatalError()<br>
>>>> > > > > static \
void<br> >>>> FatalError(JNIEnv *env,<br>
>>>> > const char<br>
>>>> > > *errrorMsg);<br>
>>>> > > > ><br>
>>>> > > > > \
SafeJNIEnv(JNIEnv*<br> >>>> jni_env, ErrorHandler<br>
>>>> > > errorHandler =<br>
>>>> > > > > FatalError);<br>
>>>> > > > > (SafeJNIEnv \
methods should<br> >>>> create<br>
>>>> > FatalOnException objects<br>
>>>> > > > passing<br>
>>>> > > > > \
errorHandler)<br> >>>> > > > \
><br> >>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > Agreed, I tried \
to keep the<br> >>>> code simple. The<br>
>>>> > concepts you<br>
>>>> > > talk about<br>
>>>> > > > > here are \
generally what I<br> >>>> reserve for when I<br>
>>>> > need it (ie<br>
>>>> > > > extensions and<br>
>>>> > > > > handling new \
cases). But a<br> >>>> lot are going to be<br>
>>>> > needed soon<br>
>>>> > > so I<br>
>>>> > > > think it<br>
>>>> > > > > is a good time to \
iron the<br> >>>> code out now on this<br>
>>>> > "simple"<br>
>>>> > > webrev.<br>
>>>> > > > ><br>
>>>> > > > > So done for \
this.<br> >>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > - \
FatalOnException is used<br> >>>> in SafeJNIEnv<br>
>>>> > methods as<br>
>>>> > > > > \
FatalOnException<br> >>>> marker(this, "msg");<br>
>>>> > > > > ret = \
<JNI call><br> >>>> > > > \
> <br> >>>> (optional)marker.check_for_null(ret);<br>
>>>> > > > > return \
ret;<br> >>>> > > > > But \
actually I'd call it<br> >>>> something like<br>
>>>> > > JNICallResultVerifier and<br>
>>>> > > > > create<br>
>>>> > > > > the object after \
JNI call. like<br> >>>> > > > \
> ret = <JNI call><br> >>>> > > \
> > <br> >>>> JNICallResultVerifier(this, \
"msg")<br> >>>> > > > \
> (optional).notNull(ret);<br> >>>> > \
> > > return ret;<br> >>>> > \
> > > or even (if you like such<br> >>>> \
syntax sugar) you<br> >>>> > can define<br>
>>>> > > > > \
template<typename T><br> >>>> > > \
> > T resultNotNull(T result) {<br> >>>> > \
> > > notNull(result);<br> >>>> \
> > > > return result;<br> >>>> \
> > > > }<br> >>>> > \
> > > and do<br> >>>> > > \
> > ret = <JNI call><br> >>>> > \
> > > return<br> >>>> \
JNICallResultVerifier(this,<br> >>>> > > \
"msg").resultNotNull(ret);<br> >>>> > \
> > ><br> >>>> > > \
> ><br> >>>> > > > \
> So I renamed FatalOnException<br> >>>> to now being<br>
>>>> > JNIVerifier.<br>
>>>> > > > ><br>
>>>> > > > > Though I like it, \
I don't<br> >>>> think we can do it,<br>
>>>> > except if we<br>
>>>> > > do it<br>
>>>> > > > > slightly \
differently:<br> >>>> > > > > \
I'm trying to solve two<br> >>>> problems with one \
stone:<br> >>>> > > > > - How \
to check for returns<br> >>>> of JNI calls in<br>
>>>> > the way that is<br>
>>>> > > > done here<br>
>>>> > > > > - How to \
remove<br> >>>> NSK_JNI_VERIFY* (from<br>
>>>> > > nsk/share/jni/jni_tools)<br>
>>>> > > > ><br>
>>>> > > > > However, the \
NSK_JNI_VERIFY<br> >>>> need to start a<br>
>>>> > tracing system<br>
>>>> > > before<br>
>>>> > > > the<br>
>>>> > > > > call to JNI, so \
it won't work<br> >>>> this way. (Side<br>
>>>> > question<br>
>>>> > > would be<br>
>>>> > > > do we<br>
>>>> > > > > still care about \
the tracing<br> >>>> in NSK_JNI_VERIFY,<br>
>>>> > if we<br>
>>>> > > don't then<br>
>>>> > > > your<br>
>>>> > > > > solution works \
well in most<br> >>>> situations).<br>
>>>> > > > ><br>
>>>> > > > > My vision or \
intuition is<br> >>>> that we would throw a<br>
>>>> > template<br>
>>>> > > at some<br>
>>>> > > > point<br>
>>>> > > > > on SafeJNIEnv to \
handle both<br> >>>> cases and have<br>
>>>> > JNIVerifier<br>
>>>> > > become a<br>
>>>> > > > > specialization in \
certain<br> >>>> cases and something<br>
>>>> > different<br>
>>>> > > for the<br>
>>>> > > > > NSK_JNI_VERIFY \
case (or have<br> >>>> a different<br>
>>>> > constructor to enable<br>
>>>> > > > tracing).<br>
>>>> > > > > But for now, I \
offer the<br> >>>> implementation that does:<br>
>>>> > > > ><br>
>>>> > > > > jclass<br>
>>>> SafeJNIEnv::GetObjectClass(jobject obj) {<br>
>>>> > > > > \
JNIVerifier<jclass> marker(this,<br> >>>> > \
"GetObjectClass");<br> >>>> > > \
> > return<br> >>>> > \
marker.ResultNotNull(_jni_env->GetObjectClass(obj));<br> >>>> \
> > > > }<br> >>>> > \
> > ><br> >>>> > > \
> > and:<br> >>>> > > > \
><br> >>>> > > > > void<br>
>>>> SafeJNIEnv::SetObjectField(jobject obj,<br>
>>>> > jfieldID<br>
>>>> > > field, jobject<br>
>>>> > > > > value) {<br>
>>>> > > > > \
JNIVerifier<> marker(this,<br> >>>> \
"SetObjectField");<br> >>>> > > \
> > _jni_env->SetObjectField(obj,<br> >>>> \
field, value);<br> >>>> > > > \
> }<br> >>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > - you added \
#include<br> >>>> <memory> in the test<br>
>>>> > (and you have to<br>
>>>> > > > add it to<br>
>>>> > > > > every test).<br>
>>>> > > > > It would be \
simpler to add<br> >>>> the include to<br>
>>>> > > SafeJNIEnv.hpp and<br>
>>>> > > > define<br>
>>>> > > > > typedef<br>
>>>> std::unique_ptr<SafeJNIEnv><br>
>>>> > SafeJNIEnvPtr;<br>
>>>> > > > > Then each in the \
test methods:<br> >>>> > > > > \
SafeJNIEnvPtr env(new<br> >>>> SafeJNIEnv(jni_env));<br>
>>>> > > > > or you can \
add<br> >>>> > > > > static \
SafeJNIEnv::SafeJNIEnvPtr<br> >>>> > wrap(JNIEnv* \
jni_env,<br> >>>> > > > \
ErrorHandler<br> >>>> > > > > \
errorHandler = fatalError)<br> >>>> > > \
> > and get<br> >>>> > > > \
> SafeJNIEnvPtr env =<br> >>>> > \
SafeJNIEnv::wrap(jni_env);<br> >>>> > > \
> ><br> >>>> > > > \
><br> >>>> > > > > Done, I \
like that, even less<br> >>>> code change to<br>
>>>> > tests then.<br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > - it would be \
better to wrap<br> >>>> internal classes<br>
>>>> > > > (FatalOnException) \
into<br> >>>> > > > > unnamed \
namespace - this<br> >>>> helps to avoid<br>
>>>> > conflicts with<br>
>>>> > > other<br>
>>>> > > > classes)<br>
>>>> > > > ><br>
>>>> > > > > -<br>
>>>> FatalOnException::check_for_null(void* ptr)<br>
>>>> > > > > should be<br>
>>>> > > > ><br>
>>>> FatalOnException::check_for_null(const<br>
>>>> > void* ptr)<br>
>>>> > > > > And calling the \
method you<br> >>>> don't need<br>
>>>> > reinterpret_cast<br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > Also done, it got \
renamed to<br> >>>> ResultNotNull, is<br>
>>>> > using a<br>
>>>> > > template<br>
>>>> > > > now, but<br>
>>>> > > > > agreed, that \
worked.<br> >>>> > > > > \
Thanks again for the review,<br> >>>> > > \
> > Jc<br> >>>> > > > \
><br> >>>> > > > ><br>
>>>> > > > > --alex<br>
>>>> > > > ><br>
>>>> > > > ><br>
>>>> > > > > On 09/18/2018 \
11:07, JC<br> >>>> Beyler wrote:<br>
>>>> > > > > > Hi \
David,<br> >>>> > > > > \
><br> >>>> > > > > > \
Thanks for the quick<br> >>>> review and<br>
>>>> > thoughts. I have<br>
>>>> > > now a new<br>
>>>> > > > > version here<br>
>>>> > > > > > that \
addresses your comments:<br> >>>> > > \
> > ><br> >>>> > > > \
> > Webrev:<br> >>>> > > ><br>
>>>> ><br>
>>>> <a \
href="http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.01/</a><br> \
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br> \
>>>> > <br> >>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br> \
>>>> > > <br> >>>> \
<<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br>
>>>> > > ><br>
>>>> > <br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br> \
>>>> > > > ><br> \
>>>> > > <br> >>>> \
<<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br>
>>>> > > > > ><br>
>>>> > > <br>
>>>> <<a \
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.01/</a>><br> \
>>>> > > > > ><br> \
>>>> > <br> >>>> Bug:<a \
href="https://bugs.openjdk.java.net/browse/JDK-8210842" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8210842</a><br> \
>>>> > > > > ><br> \
>>>> > > > > > I've \
also inlined my<br> >>>> answers/comments.<br>
>>>> > > > > ><br>
>>>> > > > > ><br>
>>>> > > > > ><br>
>>>> > > > > > > \
I've slowly<br> >>>> started working on<br>
>>>> > JDK-8191519<br>
>>>> > > > > > \
><br> >>>> > > <br>
>>>> <<a \
href="https://bugs.openjdk.java.net/browse/JDK-8191519" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8191519</a>>.<br> \
>>>> > > > > However \
before<br> >>>> > > > > > \
> starting to really<br> >>>> refactor all<br>
>>>> > the tests, I<br>
>>>> > > > thought I'd<br>
>>>> > > > > get a few<br>
>>>> > > > > > > \
opinions. I am<br> >>>> working on<br>
>>>> > internalizing the<br>
>>>> > > error<br>
>>>> > > > handling<br>
>>>> > > > > of JNI<br>
>>>> > > > > > \
calls<br> >>>> > > > > > \
> via a SafeJNIEnv<br> >>>> class that<br>
>>>> > redefines all<br>
>>>> > > the JNI<br>
>>>> > > </blockquote></div><br \
clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" \
data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic