Harsha, EXCEPTION_CHECK_AND_FREE macro could be used at ll.76 after FindClass
-Dmitry On 2016-09-06 08:47, Harsha Wardhana B wrote: > Hi David, > > Please find revised webrev. > > http://cr.openjdk.java.net/~hb/8161448/webrev.03/ > > -Harsha > > On Thursday 01 September 2016 03:31 PM, David Holmes wrote: >> On 1/09/2016 6:05 PM, Harsha Wardhana B wrote: >>> Hi David, >>> >>> On Thursday 01 September 2016 01:14 PM, David Holmes wrote: >>>> Hi Harsha, >>>> >>>> Sorry these style issues are proving to be so painful, normally there >>>> would be more direct guidance from an existing team member. >>>> >>>> On 30/08/2016 4:51 PM, Harsha Wardhana B wrote: >>>>> Hello, >>>>> >>>>> Please find below webrev addressing David's and Dmitry's comments. >>>>> >>>>> http://cr.openjdk.java.net/~hb/8161448/webrev.02/ >>>> >>>> The idiomatic way we write such macros is as follows: >>>> >>>> #define EXCEPTION_CHECK_AND_FREE(x) \ >>>> do { \ >>>> if ((*env)->ExceptionCheck(env)) { \ >>>> free(x); \ >>>> return NULL; \ >>>> } \ >>>> } while (false) >>> I am aware of this style but I wasn't aware what style should be used >>> (BTW last line should be 'while(0)'). I didn't want to do anything fancy >>> and end up sending one more webrev and hence I followed the simplest way >>> to write a multi-line macro. >>> Now if there isn't anything wrong, I think we should leave it as is >>> until coding guidelines are put in place. I am facing same problem in >>> another issue where I have sent multiple webrev fixing only nits. It is >>> turning out to be very painful. >> >> I understand and it is unfortunate that there has not been more >> definitive guidance here. But it is better to adopt established style >> and get used to using it. If you form is left in then you need spaces >> before trailing \ - another style nit, sorry. >> >>>> >>>> --- >>>> >>>> 109 if (obj == NULL) { >>>> 110 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>> 111 } >>>> >>>> Checking both NULL and for exceptions is redundant as previously >>>> pointed out. Either leave this code as it was: >>> Yes. it may be redundant but it is harmless. Does this really need to be >>> changed? >> >> Yes it breeds confusion to see multiple checks that are unnecessary >> (and we're trying to address this with a lot of the JNI using code in >> the VM at the moment). In this case it is even worse because without >> knowing the exception check must be true and so "return NULL" will >> always execute, this code has the appearance of being able to continue >> if obj == NULL! >> >> Sorry. And thanks. >> >> David >> ----- >> >>>> >>>> 91 if (obj == NULL) { >>>> 92 free(dcmd_arg_info_array); >>>> 93 return NULL; >>>> 94 } >>>> >>>> or else just replace it with: >>>> >>>> 109 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>> >>>> The same for the block here: >>>> >>>> 201 if (obj == NULL){ >>>> 202 EXCEPTION_CHECK_AND_FREE(dcmd_info_array); >>>> 203 } >>>> >>>> >>>> Thanks, >>>> David >>>> ----- >>>> >>>>> -Harsha >>>>> >>>>> On Tuesday 30 August 2016 09:46 AM, David Holmes wrote: >>>>>> On 30/08/2016 2:06 PM, Harsha Wardhana B wrote: >>>>>>> Error checking might distract the main flow of code but it would be >>>>>>> far-fetched to call that it obfuscates the code. Ideally error >>>>>>> checking >>>>>> >>>>>> You claimed macros obfuscate so the same word seemed appropriate. I >>>>>> don't necessarily equate readability with obfuscation. >>>>>> >>>>>>> could have been made a function (inline if possible) instead of a >>>>>>> macro >>>>>>> but since it is context sensitive in this case (have to free >>>>>>> variables >>>>>>> depending on context) , doing so would obfuscate the code even more. >>>>>>> >>>>>>> I tried to separate it into a function and the code looks more >>>>>>> uglier. >>>>>> >>>>>> I was envisaging something like: >>>>>> >>>>>> jstring jname, jdesc,jtype,jdefStr; >>>>>> jname = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].name); >>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>>>> jdesc = >>>>>> (*env)->NewStringUTF(env,dcmd_arg_info_array[i].description); >>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>>>> jtype = (*env)->NewStringUTF(env,dcmd_arg_info_array[i].type); >>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>>>> jdefStr = (*env)->NewStringUTF(env, >>>>>> dcmd_arg_info_array[i].default_string); >>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array); >>>>>> >>>>>> Of course if this were C++ code instead of C there would be better >>>>>> techniques for dealing with the free'ing. >>>>>> >>>>>> Cheers, >>>>>> David >>>>>> >>>>>> >>>>>>> -Harsha >>>>>>> On Tuesday 30 August 2016 09:26 AM, David Holmes wrote: >>>>>>>> On 30/08/2016 1:47 PM, Harsha Wardhana B wrote: >>>>>>>>> Hi Dmitry, >>>>>>>>> >>>>>>>>> I am not sure how using macro will help in this context. As far >>>>>>>>> as I >>>>>>>>> know, macros must be sparingly used as they are error-prone, >>>>>>>>> obfuscate >>>>>>>>> the code and hard to debug. Most of best coding practice for c/c++ >>>>>>>>> (inluding Scott Myers Effective C++ ) advocate using macros only >>>>>>>>> if it >>>>>>>>> is absolutely needed. >>>>>>>> >>>>>>>> Macros are used extensively throughout hotspot and the JDK native >>>>>>>> code. That isn't to say that all such macro uses are good (we have >>>>>>>> bad >>>>>>>> code too). However macros make sense where you want to avoid code >>>>>>>> duplication and improve readability - as is the case here. It is >>>>>>>> quite >>>>>>>> common to "hide" error checking logic behind a macro because it is >>>>>>>> the >>>>>>>> error-checking logic that obfuscates the code. >>>>>>>> >>>>>>>> David >>>>>>>> ----- >>>>>>>> >>>>>>>>> -Harsha >>>>>>>>> >>>>>>>>> >>>>>>>>> On Monday 29 August 2016 02:00 PM, Dmitry Samersoff wrote: >>>>>>>>>> Harsha, >>>>>>>>>> >>>>>>>>>> I'm for macro. >>>>>>>>>> >>>>>>>>>> You can define a macro right before a place where you uses it and >>>>>>>>>> undef >>>>>>>>>> it when you don't need it anymore. >>>>>>>>>> >>>>>>>>>> -Dmitry >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2016-08-25 11:19, Harsha Wardhana B wrote: >>>>>>>>>>> Hello All, >>>>>>>>>>> >>>>>>>>>>> Please find below the revised webrev below. >>>>>>>>>>> >>>>>>>>>>> http://cr.openjdk.java.net/~hb/8161448/webrev.01/ >>>>>>>>>>> >>>>>>>>>>> Regards >>>>>>>>>>> >>>>>>>>>>> Harsha >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wednesday 24 August 2016 11:48 AM, Harsha Wardhana B wrote: >>>>>>>>>>>> Hi David, >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tuesday 23 August 2016 12:41 PM, David Holmes wrote: >>>>>>>>>>>>> Hi Harsha, >>>>>>>>>>>>> >>>>>>>>>>>>> On 22/08/2016 6:30 PM, Harsha Wardhana B wrote: >>>>>>>>>>>>>> Hello All, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Please review the below parfait fixes for >>>>>>>>>>>>>> DiagnosticCommandImpl.c >>>>>>>>>>>>>> >>>>>>>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8161448 >>>>>>>>>>>>>> >>>>>>>>>>>>>> webrev : http://cr.openjdk.java.net/~hb/8161448/webrev.00/ >>>>>>>>>>>>> Looks functionally correct, but I wouldn't complain if you >>>>>>>>>>>>> wanted to >>>>>>>>>>>>> use a macro to reduce the duplication and verbosity. Even the: >>>>>>>>>>>>> >>>>>>>>>>>>> 109 if (obj == NULL) { >>>>>>>>>>>>> 110 free(dcmd_arg_info_array); >>>>>>>>>>>>> 111 return NULL; >>>>>>>>>>>>> >>>>>>>>>>>>> can be replaced by an exception-check as that is the only time >>>>>>>>>>>>> JNU_NewObjectByName can return NULL. >>>>>>>>>>>> I am not sure if using macro here is good practice since it >>>>>>>>>>>> will be >>>>>>>>>>>> specific to the function and it will encapsulate the local >>>>>>>>>>>> variable >>>>>>>>>>>> within it. Also, it will reduce code readability. Hence I would >>>>>>>>>>>> like >>>>>>>>>>>> to leave it as is. >>>>>>>>>>>>> I also noticed an issue that was flagged in some other JNI >>>>>>>>>>>>> code >>>>>>>>>>>>> lately: >>>>>>>>>>>>> >>>>>>>>>>>>> 213 if (obj == NULL) { >>>>>>>>>>>>> 214 free(dcmd_info_array); >>>>>>>>>>>>> 215 JNU_ThrowOutOfMemoryError(env, 0); >>>>>>>>>>>>> 216 return NULL; >>>>>>>>>>>>> >>>>>>>>>>>>> If obj == NULL then there is already a pending exception >>>>>>>>>>>>> and we >>>>>>>>>>>>> should not be throwing OOME. >>>>>>>>>>>>> >>>>>>>>>>>> Sure. This needs to be fixed. >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> David >>>>>>>>>>>>> >>>>>>>>>>>>>> Regards >>>>>>>>>>>>>> >>>>>>>>>>>>>> Harsha >>>>>>>>>>>>>> >>>>>>>>>>>> Harsha >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>> >>> Regards >>> Harsha > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
