On Fri, 28 Jun 2024 18:02:28 GMT, Kevin Walls <[email protected]> wrote:
>> Sebastian Lövdahl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add test for the elevated privileges case
>
> src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java:
>
> It's going to be mostly style nits. 8-)
> I realise some of this was passed back and forth above (so apologies to you
> and to Larry for being a pain).
>
>
> line 281 line break before IOException
>
>
> 313
> There is no hard limit on line length, but when you add new longest lines
> it's probably diverged from the file's style.
> Could you make some of the new comments more terse?
>
>
> A lot of the new code is double-spaced, which is unusual for the file for
> HotSpot/JDK as a whole.
> Is it still readable to you if we delete at least these empty lines:
> 274, 276, 286, 296, 303, 312, 317, 319, 324, 326
>
> It may not quite get findTargetProcessTmpDirectory to fit on screen, but it
> will help. 8-)
Hi again @kevinjwalls,
Thanks a lot for the review! And no worries :) I'm happy to adapt the code
style to match the rest of the file and the JDK in general.
I pushed some changes in 8b200fc7f1d4bb00a4f18c90cd8e36374ade98ba. I removed a
lof of newlines and tried to make the comments more terse. The two `throw new
IOException(String.format("unable to attach, %s non-existent! process: %d
terminated", procPidPath, pid));` lines still stand out in terms of line
length, should I wrap them?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2199413193
