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.
> 
> 
> 
> 

Reply via email to