Hi Mandy, Here is updated patch http://cr.openjdk.java.net/~anazarov/8178323/webrev.02/webrev/ <http://cr.openjdk.java.net/~anazarov/8178323/webrev.02/webrev/>
— Thanks, Andrey > On 10 Apr 2017, at 23:50, Mandy Chung <mandy.ch...@oracle.com > <mailto:mandy.ch...@oracle.com>> wrote: > > >> On Apr 10, 2017, at 12:30 PM, Andrey Nazarov <andrey.x.naza...@oracle.com >> <mailto:andrey.x.naza...@oracle.com>> wrote: >> >> Mandy, thanks for review. I’ve updated patch >> http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ >> <http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/> >> > > I suggest to add m4/p4.S to make it clear it’s a service type and m4 provides > p4.S with p4.Impl. > noOneUsesProvider will call —suggest-providers p4.S instead. > > 239 static JLink run(boolean expectSuccess, String... options) { > Instead of adding a new parameter, what about adding a new > runWithNonZeroExitCode method? > >> See my comments inline >>> >>> >>> line 159-161: formatting nit: can you add spaces to align with the first >>> argument in line 158. Same comment to SuggestProviders.java line 180-182 >>> an 198-200. >> Fixed > > line 216-219. > >> Actually there are two issues here: >> https://bugs.openjdk.java.net/browse/JDK-8178404 >> <https://bugs.openjdk.java.net/browse/JDK-8178404> >> https://bugs.openjdk.java.net/browse/JDK-8178405 >> <https://bugs.openjdk.java.net/browse/JDK-8178405> > Not really a bug but it’d be helpful to show that the provider exists but the > service is not used in this case. > Mandy >
