On 12/21/18 2:31 PM, Chris Plummer wrote:
Hi Gary,
This is much better. Just one question and a couple of minor comments:
The original code had the following:
225 context.options.string = (char*)malloc(len + 2);
...
231 strncpy(context.options.string, options, len);
232 context.options.string[len] = '\0';
233 context.options.string[len+1] = '\0';
Do you know why it was placing two NULLs at the end (and the first was
unnecessary since strncpy already did that)?
I choose to believe the author was confused.
243 *value++ = '\0';
This should be rewritten in a manner that make it clearer what it
does. At first glance you have to ask yourself whether it is setting
*value or *(value++). Then hopefully you eventually clue in that they
are both the same since "value++" evaluates to "value". I think the
following would be better:
*value = '\0';
value++;
I believe it is a common idiom for C programs, to set a null and
increment a pointer.
If you think it is confusing I can change it.
235 /* Create a temporay copy of the options string to be
tokenized. */
"temporay" typo.
Done.
I did run into solaris and windows build failures, because the index()
prototype is in strings.h on other platforms.
I'll also try substituting strchr() so we can just include string.h
I've been checking other places in the jdk sources and noticed
checks for _MSC_VER, _WIN32, defined(__linix__), and NEED_BSD_STRINGS
when including strings.h
I'll post a fresh webrev when I get clean builds.
...
thanks,
Chris
On 12/21/18 10:52 AM, Gary Adams wrote:
Here is a first pass at a replacement parser for jvmti test options.
Webrev: http://cr.openjdk.java.net/~gadams/8211343/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8211343
Testing is in progress. Need to check out more platforms.
On local linux testing one jvmti test failed SetNativeMethodPrefix001
which passed an empty options string.
...
