On Mon, 30 Oct 2023 09:55:35 GMT, Johan Sjölen <jsjo...@openjdk.org> wrote:
>> Thomas Stuefe has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix Windows build
>
> 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;
> }
> }
Ouch, I just realized that we can't differentiate between being provided with
the literal number 2 and `MemStatAction::print` anymore since you moved the
literal number parsing into this function. That means that we can't set
`print_final_memstat_report` after returning from this function.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16335#discussion_r1375979272
