HI Mandy, I’ve update patch http://cr.openjdk.java.net/~anazarov/8178323/webrev.03/webrev/ <http://cr.openjdk.java.net/~anazarov/8178323/webrev.03/webrev/> I’ve removed some tests since you pushed them with fix for https://bugs.openjdk.java.net/browse/JDK-8178404
—Andrey > On 11 Apr 2017, at 12:46, Andrey Nazarov <andrey.x.naza...@oracle.com> wrote: > > 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> 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 >> >
