On Thu, 4 Jul 2024 06:36:41 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> 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 > >> 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 Thank you both for the reviews! @tstuefe @kevinjwalls ------------- PR Comment: https://git.openjdk.org/jdk/pull/19776#issuecomment-2209425133
