RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests [v3]
Severin Gehwolf
sgehwolf at openjdk.org
Thu Dec 15 09:48:08 UTC 2022
On Wed, 14 Dec 2022 19:28:32 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:
>> Hi!
>>
>> Citing the ticket directly:
>>
>> The function subsystem_file_line_contents does the simple job of parsing lines in a file, but does so in a fairly complex way. This function can be simplified. The function also has a surprising API, as you need to know that in one case the format string needs to take 2 specifiers, instead of 1.
>>
>> Some more context:
>>
>> `subsystem_file_line_contents` either parses files that look like this:
>>
>>
>> one_value
>>
>>
>> Called as: `subsystem_file_line_contents(ctrl, fname, nullptr, "%s")`
>>
>> Or like this:
>>
>>
>> key1 val1
>> key2 val2
>>
>>
>> Called as: `subsystem_file_line_contents(ctrl, fname, "key1", "%s %s")`
>>
>> The API for the key/value case is changed to: `subsystem_file_line_contents(ctrl, fname, "key1", "%s")`. Note: `"%s"`, not `"%s %s"`.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
>
> Add two more test cases
Looks fine, thanks!
test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 100:
> 98:
> 99: s[0] = '\0';
> 100: fill_file(test_file, "foo\tbar");
Since this is in the multi-line test (`SubSystemFileLineContentsMultipleLines`), perhaps actually make it multi-line? Something like `fill_file(test_file, "foo\tbar\nfoot car");`
test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 143:
> 141: err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%*s %d", &x);
> 142: EXPECT_EQ(err, 0);
> 143: EXPECT_EQ(x, 10001);
These are single-line tests, perhaps move them to `SubSystemFileLineContentsSingleLine`?
-------------
Marked as reviewed by sgehwolf (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11667
More information about the hotspot-runtime-dev
mailing list