On Sun, 2019-05-12 at 09:35 +0100, Alan Bateman wrote: > On 08/05/2019 15:37, sgehw...@redhat.com wrote: > > : > > > 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. > > OK. Done. > Good, that should help in the event that objdump produces an error. > > > > > 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. > > Answered. It must never be null currently. Once it is, it's a bug. > > Hence, IllegalStateException being thrown. > It might be better to throw InternalError as ISE suggests there is a > state where it could succeed. >
OK. I'll update this to throw InternalError before I push and jdk/submit comes back clean. > > : > > FWIW, since DefaultStripDebugPlugin is being run via IntegrationTest > > running the real plugins is covered too. > > > Thanks for the explanation. I looked through 10/webrev and I think this > patch is ready to go. Excellent, thank you! Cheers, Severin
