RFR: 8360178: TestArguments.atojulong gtest has incorrect format string [v2]

[Kim Barrett]

On Mon, 23 Jun 2025 07:31:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   back out jio_snprintf => os::snprintf
>
> test/hotspot/gtest/runtime/test_arguments.cpp line 32:
> 
>> 30: #include <errno.h>
>> 31: 
>> 32: #include "unittest.hpp"
> 
> Is there a rule/guideline that says this should be placed here?

The unittest.hpp include is clearly improperly ordered, so should be moved.
The question is, where to. It's very common for it to be the last include.
And the header itself suggests it is supposed to be last:
https://github.com/openjdk/jdk/blob/0514cee6c884b6d31735551b8a3ce7a2be661094/test/hotspot/gtest/unittest.hpp#L46-L47

My recollection is that there used to be documentation (possibly internal or
preliminatry during development of gtest support) that it needed to be last.
But that doesn't seem to have made it into the public documentation. It was
left out of the style guide include order rules because it was still under
discussion / investigation.

> test/hotspot/gtest/runtime/test_arguments.cpp line 51:
> 
>> 49:     char buf[1024];
>> 50:     int ret = os::snprintf(buf, sizeof(buf), "%s=%s", name, value);
>> 51:     if ((ret > 0) && ((size_t)ret < sizeof(buf))) {
> 
> `jio_snprintf` already handled the buffer-too-small case by returning -1. I don't see the reason for changing the test to use `os::snprintf` when arguments.cpp uses `jio_snprintf`.

@coleenp and I have been discussing JDK-8198918, including `jio_snprintf`, and
wanting to move away from it, at least in VM code. Since this change is in
preparation for that issue, I included a bit of "cleanup". But yeah, that
should probably wait for further discussion. I've backed out the changes
related to that.

> test/hotspot/gtest/runtime/test_arguments.cpp line 63:
> 
>> 61:   int ret = os::snprintf(ullong_max, sizeof(ullong_max), "%llu", ULLONG_MAX);
>> 62:   ASSERT_LT(0, ret);
>> 63:   ASSERT_LT(ret, (int)sizeof(ullong_max));
> 
> This is just a sanity check right? Maximum uint64_t has 20 digits.

Backed out - see other reply.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25927#discussion_r2164307139
PR Review Comment: https://git.openjdk.org/jdk/pull/25927#discussion_r2164307055
PR Review Comment: https://git.openjdk.org/jdk/pull/25927#discussion_r2164308328