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