Hi Phil, Thanks for the review. I have raised a CSR for the same. Please review it. Here is the link: https://bugs.openjdk.java.net/browse/JDK-8221680 <https://bugs.openjdk.java.net/browse/JDK-8221680>
Thanks, Krishna > On 29-Mar-2019, at 2:27 AM, Philip Race <philip.r...@oracle.com> wrote: > > > Ok, fair enough > > +1 > > -phil. > > On 3/27/19, 11:21 PM, Krishna Addepalli wrote: >> >> No it is not. When I’m adding “+1” and “+5”, it simply adds that many number >> of characters to the size. >> The sizeof(char) can be more than 1, and this statement at line 51 (filePath >> = new char[filePathSize]), takes care of allocating sufficient number of >> bytes, which accommodate “filePathSize” number of characters. >> The problem is with memcpy and memset functions, which expect the size of >> the buffer to be provided in terms of bytes, which is why, we need the >> multiplier only in those functions. >> >> Even I’m in favor of getting rid of them, but unfortunately I cannot >> std::string in this case, which would have vastly simplified the code, and >> avoided this manual calculation of sizes, but that results in compiler >> errors. >> >> Thanks, >> Krishna >> >> From: Phil Race >> Sent: Thursday, March 28, 2019 3:11 AM >> To: Krishna Addepalli <krishna.addepa...@oracle.com> >> <mailto:krishna.addepa...@oracle.com>; swing-dev@openjdk.java.net >> <mailto:swing-dev@openjdk.java.net> >> Subject: Re: <Swing Dev> [13] RFR: JDK-8219914: Change the environment >> variable for Java Access Bridge logging to have a directory. >> >> Hi, >> >> It is still inconsistent. the "+1" and "+5" logic relies on the answer to >> sizeof(char) being 1. >> So either get rid of all those calls or use sizeof('/') and sizeof(".logX"); >> instead of 1 and 5. >> >> I'd get rid of them .... >> >> -phil >> >> On 3/27/19 1:41 AM, Krishna Addepalli wrote: >> Hi Phil, >> >> Thanks for the review. >> I have changed the variable to “JAVA_ACCESSBRIDGE_LOGDIR”. >> >> Yes, ‘/’ file separator character works on windows. I have used it in the >> past and have also currently tested it on my machine and it works. >> >> I have added the multiplier “sizeof(char)” for all memcpy and memset lines >> in the code, to keep it consistent. Thanks for pointing that out. >> >> Here is the link to the webrev: >> http://cr.openjdk.java.net/~kaddepalli/8219914/webrev01/ >> <http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev01/> >> >> Thanks, >> Krishna >> >> From: Phil Race >> Sent: Tuesday, March 26, 2019 10:42 PM >> To: Krishna Addepalli <krishna.addepa...@oracle.com> >> <mailto:krishna.addepa...@oracle.com>; swing-dev@openjdk.java.net >> <mailto:swing-dev@openjdk.java.net> >> Subject: Re: <Swing Dev> [13] RFR: JDK-8219914: Change the environment >> variable for Java Access Bridge logging to have a directory. >> >> Can we just call it JAVA_ACCESSBRIDGE_LOGDIR ? >> >> >> filePath[envFilePathLength] = '/'; >> >> Is this right ? Does fopen on Windows expect this unix style separator ? >> >> >> 53 memcpy(filePath, envfilePath, envFilePathLength*sizeof(char)); >> >> 56 memcpy(filePath + envFilePathLength + 1 + fileNameLength, ".log", >> 4*sizeof(char)); >> >> Interesting that you feel it necessary to use sizeof(char) when clearly the >> whole logic, eg see : >> >> >> 50 auto filePathSize = envFilePathLength + 1 + fileNameLength + 5; >> //1 for "/", 5 for ".log" and 0; >> >> assumes it is 1 ... >> >> PrintDebugString("couldnot open file %s", filePath); >> couldnot -> could not >> >> -phil. >> >> On 3/26/19 2:45 AM, Krishna Addepalli wrote: >> Hi Phil, >> >> Per our discussion, I have changed the JAVA_ACCESSBRIDGE_LOGFILE to >> JAVA_ACCESSBRIDGE_LOGDIRECTORY to reflect that it accepts only a directory >> value in the variable. >> I have also changed the code in AccessBridgeDebug.cpp appropriately. >> So, currently, the code will look for the environment variable, which should >> contain path to the directory, and two log files namely >> “java_access_bridge.log” and “windows_access_bridge.log” will be created. >> >> Link to the JDK Issue: https://bugs.openjdk.java.net/browse/JDK-8219914 >> <https://bugs.openjdk.java.net/browse/JDK-8219914> >> Here is the webrev: http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/ >> <http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev00/> >> >> >> Thanks, >> Krishna >> >>
