On Mon, 23 Sep 2024 02:01:47 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Simon Tooke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> delete commented out code
>
> test/hotspot/gtest/runtime/test_os.cpp line 442:
>
>> 440: errno = 0;
>> 441: returnedBuffer = os::realpath(tmppath, buffer, MAX_PATH);
>> 442: EXPECT_TRUE(returnedBuffer != nullptr);
>
> Why the change?
An earlier misundertanding had me thinking the buffer allocated by realpath
could be returned. I've reverted this change.
> test/hotspot/gtest/runtime/test_os.cpp line 452:
>
>> 450: EXPECT_TRUE(returnedBuffer == nullptr);
>> 451: EXPECT_TRUE(errno == ENAMETOOLONG);
>> 452: #endif
>
> I think it would be better to increase the buffer size on macOS so this
> remains a positive test for all platforms.
We already have different behaviour flagged for Windows. If we were to
increase the buffer size, it's probably better to simply delete this test as it
would behave the same as the test immediately prior.
> test/hotspot/gtest/runtime/test_os.cpp line 460:
>
>> 458:
>> 459: /* the following tests cause an assert in fastdebug mode */
>> 460: DEBUG_ONLY(if (false)) {
>
> Suggestion:
>
> #ifndef ASSERT
>
> no need for a runtime check.
fixed.
> test/hotspot/gtest/runtime/test_os.cpp line 467:
>
>> 465:
>> 466: errno = 0;
>> 467: returnedBuffer = os::realpath(tmppath, buffer, sizeof(buffer));
>
> This is still not an EINVAL case - the buffer should be null.
> Suggestion:
>
> returnedBuffer = os::realpath(tmppath, nullptr, sizeof(buffer));
Somehow my fix for this didn't make it into the last commit. Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1791924994
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1791932505
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1791930993
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1791927384
