On Wed, 5 Mar 2025 10:29:21 GMT, Johan Sjölen <jsjo...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback - test
>
> src/hotspot/os/posix/attachListener_posix.cpp line 150:
>
>> 148: }
>> 149:
>> 150: void complete(jint res, bufferedStream* st) override;
>
> Style: Put brackets together `{}`
Fixed
> src/hotspot/os/posix/attachListener_posix.cpp line 152:
>
>> 150: void complete(jint res, bufferedStream* st) override;
>> 151:
>> 152: virtual ReplyWriter* get_reply_writer() override {
>
> Style: No need for both `virtual` and `override`.
Fixed.
> src/hotspot/os/windows/attachListener_windows.cpp line 128:
>
>> 126: if (!fSuccess) {
>> 127: log_error(attach)("pipe write error (%d)", GetLastError());
>> 128: return -1;
>
> Style wish: Could you rename `fSuccess` to something like `write_success`?
This is general style in the file (I believe it came from MSDN examples), so I
prefer to leave it as is for now
> src/hotspot/os/windows/attachListener_windows.cpp line 159:
>
>> 157: void complete(jint result, bufferedStream* result_stream) override;
>> 158:
>> 159: virtual ReplyWriter* get_reply_writer() override {
>
> Style: virtual && override unnecessary
Fixed.
> src/hotspot/share/services/attachListener.cpp line 63:
>
>> 61: class attachStream : public bufferedStream {
>> 62: AttachOperation::ReplyWriter* _reply_writer;
>> 63: bool _allow_streaming;
>
> Is this `const`?
Right. Fixed.
> src/hotspot/share/services/attachListener.cpp line 66:
>
>> 64: jint _result;
>> 65: bool _result_set;
>> 66: bool _result_written;
>
> It seems like `_result_written` implies `_result_set`? You can do this:
>
> ```c++
> enum class ResultState { Unset; Set; Written; };
> ResultState _result_state;
> jint _result;
>
>
> And then have the `ResultState` transition from `Unset` to `Set` to `Written`.
Updated
> src/hotspot/share/services/attachListener.cpp line 213:
>
>> 211: }
>> 212:
>> 213: static void get_properties(AttachOperation* op, attachStream* out,
>> Symbol* serializePropertiesMethod) {
>
> In this function you have to remember to set the result, otherwise you'll
> accidentally drop the result. If you return `jint`, you're forced to remember
> to write a `return`.
>
> I'd say that you should revert the changes for this function, and instead
> have the callers of this method set the result code.
>
> In fact, revert this kind of change for all functions in
> `AttachOperationFunctionInfo funcs[]` and have all send the return code. That
> simplifies the pattern significantly, as you only need to set the result
> early if you want to be streaming your output. That also minimizes the
> changeset, win-win.
Updated as suggested. But github considers diff for the file as "large" :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982500667
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982501693
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982528121
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982502593
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982504691
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982517135
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1982514245
