On Wed, 7 Apr 2021 17:52:16 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address lyndseyBeil, robehn and sspitsyn CR comments. > > Hi Dan, > It looks good in general. > But I think moving JVMTI error code checks to Java is a step in a wrong > direction. > First, it makes it inconsistent with all existing JVMTI tests. In this > particular case, all the complexity is moved to Java side making it > unbalanced. Also, we are working with Leonid toward creating relevant native > testing lib for serviceability/jvmti tests (created it for Loom first) which > has a support to print symbolic names (for error codes as well) if necessary. > Not sure, I understood what wrong with the native check_jvmti_status(). It > seems the id argument is not needed much and can be removed anyway. > Thanks, > Serguei Hi @sspitsyn, I rather like the much simpler agent code and it puts the majority of logic in the .java file so the reader/reviewer doesn't have to jump back and forth between the two files nearly as much. The `id` argument had to be passed to the old version of the function calls in order for the logging messages generated by the agent code to (easily) have the same thread names as the Java side logging. Once all the error checking was moved to the Java side, that made the `id` argument no longer necessary and also allowed the native `check_jvmti_status()` function (along with other functions) to be removed. So there was nothing wrong with the native `check_jvmti_status()` function; it was just made obsolete by the code migration to the Java side. I consider @robehn's idea here to be good evolution for these kinds of tests to move code from the native side, where the code is more platform dependent, to the Java side, where the code is more platform independent. I hope I have convinced you that this is a good changeset for moving forward. ------------- PR: https://git.openjdk.java.net/jdk/pull/2899
