Thank you Alan for reviewing!
Please see the comments inline.
On 11/26/19 11:54 PM, Alan Bateman wrote:
On 27/11/2019 04:39, Ivan Gerasimov wrote:
Hello!
It is a cleanup fix with mostly two kinds of changes:
- when a standard charset is specified by its name, use a
preinitialized Charset constant instead,
- replace the usage of StandardCharset.* constants with their
sun.nio.cs.* equivalents to avoid accidental early initialization of
rarely-used charsets.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8234147
WEBREV: http://cr.openjdk.java.net/~igerasim/8234147/00/webrev/
I also had to modify one regression test that relied on a private
auxiliary method, which was removed with the fix.
Mach5 control build looks green.
Would you please help review the fix?
The motivation is okay and most of the changes are okay but it does
create a needless dependency on sun.nio.* classes in places that are
unlikely (or impossible) to use during early startup. I think the
change to the JDBC CachedRow implementation, prefs, and the
jdk.net.httpserver module should be dropped as want fewer classes
outside of java.base depending on java.base internals, not more. I
also wonder about the NTLM, XML properties and a few more that
shouldn't be dependency on sun.nio.* classes if possible.
Please note that in fact sun.nio.cs.* was not used in java.net.http,
java.prefs and java.sql.rowset. I only used those sun.nio constants
inside the java.base module, exactly for the reason of avoiding
additional dependencies.
It's not clear how to distinguish the classes inside the java.base
module that do not have to depend on sun.nio.cs. If you feel strong
about NTLM and XML, I can replace sun.nio.cs.* instances with
corresponding java.charset.StandardCharsets.* constants in these classes.
Minot nit but if this is pushed then it would be good to keep the
imports consistent with the existing style where you can, e.g. I see
several files where the import sun.nio.* is put at the top of source
files that already group imports of JDK internal classes together
further down.
Thanks, done. Unfortunately, imports conventions are not very
consistent across the JDK code, so I mostly tried to preserve the
pre-existing order of imports in each file.
Mandy might want to comment on TrySetAccessibleTest. If I'm not
mistaken it should say "non-exported" rather than "exported". Also I
think we want this test to have a dependency on a few JDK internal and
private methods as possible as it's a maintenance issue to keep it up
to date (as you've experienced here with the removal of Perf.getBytes).
If I got it correct, the test was meant to access a private static
method, and after removing Perf.getBytes there were no such methods left
in the Perf class. That's why I had to pick some other method to test,
so I chose ModulePath.packageName instead.
Here's the updated webrev with reorganized imports:
http://cr.openjdk.java.net/~igerasim/8234147/01/webrev/
Thanks again!
--
With kind regards,
Ivan Gerasimov
