On Wed, 3 Jul 2024 14:31:51 GMT, Sonia Zaldana Calles <szald...@openjdk.org> wrote:
>> Hi all, >> >> This PR addresses [8332124](https://bugs.openjdk.org/browse/JDK-8332124) >> enabling jcmd to recognize options that look like help. >> >> Testing: >> - [x] Added test case passes. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with two > additional commits since the last revision: > > - Updating comments > - Enabling -help to work as well. Updating test cases Marked as reviewed by stuefe (Reviewer). > OR, avoid the allocation, we are definitely just reading the characters, so > we could use: char _rest = (char_) args.base(); (strtok_r wants a non-const > char*) > > Then OK great, done. > No, please don't do that. It is bad practice - don't get in the habit of casting away constness unless you are sure you know what happens. strtok modifies the string, that is why it needs write access, and it would destroy the string, making it unusable for the case when we have *not* a help command. Other than that, it looks okay for me and I will approve it in this form. However, if you have the time, you could rewrite the reorder function to not use strtok but to do the scanning yourself. - with a pointer pointing to the start of arguments - walk pointer for as long as there are spaces, or until \0 - then, use strncmp (notice the n) to check for the commands - then you are done. No need to copy the string, no need for strtok to filet the string in its whole glory just for most of it to be ignored later. Cheers, Thomas src/hotspot/share/services/diagnosticFramework.cpp line 427: > 425: args.print("%s", line.args_addr()); > 426: char *rest = args.as_string(); > 427: char *token = strtok_r(rest, " ", &rest); Nit, please use char* V not char *V, the former is much more common in hotspot. ------------- PR Review: https://git.openjdk.org/jdk/pull/19776#pullrequestreview-2158073557 PR Comment: https://git.openjdk.org/jdk/pull/19776#issuecomment-2208225831 PR Review Comment: https://git.openjdk.org/jdk/pull/19776#discussion_r1665190517
