On Fri, 28 Jun 2024 18:02:28 GMT, Kevin Walls <kev...@openjdk.org> 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