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>;
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
Here is the webrev:
http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/
<http://cr.openjdk.java.net/%7Ekaddepalli/8219914/webrev00/>
Thanks,
Krishna
