On 09/04/2019 10:06, Severin Gehwolf wrote:
:
Hi Severin,
This is my initial set of comments.
Thanks for the review! Latest webrev with the updates:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/09/webrev/
I went through 09/webrev and just have a few comments, mostly on
StripNativeDebugSymbolsPlugin.
stripBinary uses ProcessBuilder to launch objdump but it doesn't
redirect or read from the process streams. This could make diagnosing
problems difficult so I think it should minimally use
redirectOutput(INHERIT) and redirectError(INHERIT) so that any
output/errors can be seen by the jlink user.
At L180 there is a question to the reader on whether arg can be null. It
could be good to answer that if possible and avoid needing to think
about that.
At L357 there mix of old and new - I assume you can be replaced with
inFile.toAbsolutePath().toString();
Can you explain the mocking in DefaultStripDebugPluginTest? I think I've
missed the reason for this test.
In passing, we try to avoid the @author tag as it's so difficult to
remove them (even if a test is completely re-worked).
-Alan
