On Sat, 9 Aug 2025 13:00:50 GMT, Shaojin Wen <s...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert the last commit: faa5d24d831
>>
>> The `send()`-vs-`sendAsync()` discrepancy will be addressed separately.
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 755:
>
>> 753: */
>> 754: public static BodyPublisher ofFileChannel(FileChannel channel,
>> long offset, long length) throws IOException {
>> 755: Objects.requireNonNull(channel, "channel");
>
> public FileChannelPublisher(FileChannel channel, long offset, long length)
> throws IOException {
> this.channel = Objects.requireNonNull(channel, "channel");
> // ...
>
> The FileChannelPublisher constructor already has a null value check for the
> parameter channel, which is redundant
We should raise an exception at the API boundary, hence this check. Why didn't
I validate `offset` and `length` here too? Because doing so would expose
implementation details in the interface. Why did I add an extra null check in
`FCP::new`? That's also an API boundary, but an internal one. Naturally, these
practices can vary depending on performance considerations.
> src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java line
> 368:
>
>> 366:
>> 367: public static IllegalArgumentException newIAE(String message,
>> Object... args) {
>> 368: return new IllegalArgumentException(format(message, args));
>
> Suggestion:
>
> return new IllegalArgumentException(message.formatted(args));
While I agree with your reasoning, it is out of the scope of this work.
For the record, I really like `String::formatted`! Yet one should try to avoid
it if the associated changes might need to be backported. 😉
> test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 105:
>
>> 103: private static final ServerRequestPair HTTP2 =
>> ServerRequestPair.of(Version.HTTP_2, false);
>> 104:
>> 105: private static final ServerRequestPair HTTPS2 =
>> ServerRequestPair.of(Version.HTTP_2, true);
>
> Suggestion:
>
> private static final ServerRequestPair
> HTTP1 = ServerRequestPair.of(Version.HTTP_1_1, false),
> HTTPS1 = ServerRequestPair.of(Version.HTTP_1_1, true),
> HTTP2 = ServerRequestPair.of(Version.HTTP_2, false),
> HTTPS2 = ServerRequestPair.of(Version.HTTP_2, true);
>
> This coding style can be considered for declaring multiple consecutive fields
> of the same type.
Applied in 91d3422. (Did not commit your suggestion due to the _"vertical
alignment"_ you employed in the variable names. It is impossible to adhere to
in the long run, see `vmIntrinsics.hpp` for an example.)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265922312
PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265925853
PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265925993
