<Swing Dev> [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge logging to have a directory.

[Philip Race]

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.addepalli at oracle.com>; 
> swing-dev at 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.addepalli at oracle.com>
>     <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
>     <mailto:swing-dev at 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
>
>         Here is the webrev:
>         http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/
>         <http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev00/>
>
>         Thanks,
>
>         Krishna
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190328/c5d07ab5/attachment.html>