Thumbs up for JShell changes. -Robert
> On Nov 28, 2017, at 3:16 AM, Lindenmaier, Goetz <goetz.lindenma...@sap.com> > wrote: > > Hi, > > I uploaded a new webrev: > http://cr.openjdk.java.net/~goetz/wr17/8189102-helpMessage/webrev.04/ > > This includes the changes > - to jshell requested by Robert > - to the test as requested by Kumar. > > See also incremental patch and the test output including all the > help messages referenced in the webrev. > > Best regards, > Goetz. > >> -----Original Message----- >> From: Kumar Srinivasan [mailto:kumar.x.sriniva...@oracle.com] >> Sent: Montag, 27. November 2017 17:43 >> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com> >> Cc: core-libs-dev@openjdk.java.net; 'compiler-...@openjdk.java.net' >> <compiler-...@openjdk.java.net>; serviceability-dev (serviceability- >> d...@openjdk.java.net) <serviceability-...@openjdk.java.net> >> Subject: Re: RFR: 8189102: All tools should support -?, -h and --help >> >> Hi, >> >> I looked at some of the components I maintain, and they look good. >> >> Wrt. the test: >> >> 1. The local variables/fields don't comply with the coding conventions, >> we have been >> following in the JDK. >> >> 2. Hmm, someone parked a .ini file in bin directory, later removed, >> perhaps on windows simply check for .exe ? Should a check exist >> to verify if file has executable permissions ?"File.canExecute". >> >> 171 // Returns true if the file is not a tool. >> 172 static boolean notATool(String file) { >> 173 file = file.toLowerCase(); >> 174 if (file.endsWith(".dll") || >> 175 file.endsWith(".map") || >> 176 file.endsWith(".pdb") || >> 177 file.equals("server")) { // server subdir on windows. >> 178 return true; >> 179 } >> 180 return false; >> 181 } >> >> >> 3. Typo in comment >> >> 201 // Check whether 'flag' appears in 'line' as a word of itself. I >> must not >> >> s/I must/It must/ >> >> >> 4. There is a method to check for isEnglishLocale in TestHelper, perhaps >> use it >> to have the test bale out in non english locales as early as possible ? >> >> 5. Has this been tested on all platforms ? do you need help testing it ? >> >> Kumar >> >> >> On 11/17/2017 3:23 AM, Lindenmaier, Goetz wrote: >>> Hi, >>> >>> please review this change. I also filed a CSR for this: >>> http://cr.openjdk.java.net/~goetz/wr17/8189102- >> helpMessage/webrev.02/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189102 >>> CSR: https://bugs.openjdk.java.net/browse/JDK-8191477 >>> >>> See the webrev for a detailed description of the changes. >>> >>> If required, I'll make break-out changes to be reviewed separately. >>> >>> I had posted a RFR before, but improved the change to >>> give a more complete overview of currently supported flags >>> for the CSR: >>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017- >> October/028615.html >>> >>> Best regards, >>> Goetz. >>> >
