Hi Kumar, I'm already looking at the issues from your other mail. Detailed comments will follow.
I'll also check and fix the new tests you report. I think we don't run the langtool tests, i.e. I know we didn't run them before the repos were merged. Obviously we should add them to our testing :) Thanks for further testing and best regards, Goetz. > -----Original Message----- > From: Kumar Srinivasan [mailto:kumar.x.sriniva...@oracle.com] > Sent: Freitag, 1. Dezember 2017 15:42 > 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, > > Besides my private comments to you, there are 2-3 internal tests > which fail. > > Have you run all the langtools tests ? There are 4 Windows tests > that fail with langtools: > > jdk/javadoc/doclet/testHelpOption/TestHelpOption.java > jdk/javadoc/tool/CheckResourceKeys.java > jdk/javadoc/tool/ToolProviderTest.java > tools/javap/InvalidOptions.java > tools/jdeps/MultiReleaseJar.java > > This changeset needs to be vetted thoroughly using internal build and test > systems. > Any Oracle engineer willing to sponsor this ? > > Kumar > > > On 11/28/2017 3:16 AM, Lindenmaier, Goetz 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> > <mailto:goetz.lindenma...@sap.com> > Cc: core-libs-dev@openjdk.java.net <mailto:core-libs- > d...@openjdk.java.net> ; 'compiler-...@openjdk.java.net > <mailto:compiler-...@openjdk.java.net> ' > <compiler-...@openjdk.java.net> <mailto:compiler- > d...@openjdk.java.net> ; serviceability-dev (serviceability- > d...@openjdk.java.net <mailto:d...@openjdk.java.net> ) > <serviceability-...@openjdk.java.net> <mailto:serviceability- > d...@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. > > > >
