On Thu, 26 Oct 2023 16:17:14 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> When using 'MemStat' CompileCommand, we accidentally register the command if
>> an invalid suboption had been specified. Fixed, added regression test
>> (verified).
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix Windows build
Hi,
This looks good to me, I've got a couple of comments (one is just a nit). I'll
leave it to the compiler folks for approval.
src/hotspot/share/compiler/compilerOracle.cpp line 654:
> 652: IF_ENUM_STRING("print", {
> 653: value = (uintx)MemStatAction::print;
> 654: print_final_memstat_report = true;
Pre-existing/nit: Setting a member variable is not part of parsing the `uintx`
value, should be done by caller.
Performing this refactoring would also enable us to to have a loop like this
(`Pair` is available in `utilities/pair.hpp`):
```c++
constexpr const int len = 2;
Pair<const char*, MemStatAction> actions[len] = {
Pair<const char*, MemStatAction>("collect", MemStatAction::collect),
Pair<const char*, MemStatAction>("print",MemStatAction::print)
};
for (int i = 0; i < len; i++) {
auto p = actions[i];
if (strncmp(line, p.first, strlen(p.first)) == 0) {
value = p.second;
return true;
}
}
test/hotspot/jtreg/compiler/compilercontrol/commands/MemStatTest.java line 3:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> 3: * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
Thanks! But I think we're OK with Redhat taking credit for this one :).
-------------
PR Review: https://git.openjdk.org/jdk/pull/16335#pullrequestreview-1703695585
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375945596
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375968556
