Harsha, OK. The fix looks good for me.
-Dmitry On 2016-09-06 09:49, Harsha Wardhana B wrote: > Dmitry, > > The scope of the issue was to fix parfait warnings though I have gone to > some extent to refactor the file. > > Agree that macro can be used, but we have to stop here and raise a new > issue to carry-on with the refactoring process. > > Thanks > > Harsha > > On Tuesday 06 September 2016 12:03 PM, Dmitry Samersoff wrote: >> 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.
