Hi Igor,
It's updated in place
http://cr.openjdk.java.net/~mli/8186610/webrev.01/, please help to
review again/./
Thank you
-Hamlin
On 2018/10/12 2:51 AM, Igor Ignatyev wrote:
Hi Hamlin,
the key point here, ModuleTargetHelper.java is not a "library" class,
it's a part of tools/jlink/plugins/SystemModuleDescriptors/ tests.
thanks for updating webrev, I have one nit --- given ModuleUtils is
the only class in jdk/test/lib/module package, I doubt we need to
introduce this package, ModuleUtils can be put into j.t.l.util package.
-- Igor.
On Oct 10, 2018, at 11:31 PM, Hamlin Li <huaming...@oracle.com
<mailto:huaming...@oracle.com>> wrote:
Thank you clarifying Igor.
Moving ModuleTargetHelper to local folder has a drawback: it's hard
for future maintainer to found it if they need to use it in other
places, that make it an "*invisible*" library class.
Although I don't fully agree with you, I have updated the webrev as
you suggested: http://cr.openjdk.java.net/~mli/8186610/webrev.01/
Thank you
-Hamlin
On 2018/10/11 11:38 AM, Igor Ignatyev wrote:
b/c this will make test library modularization[1] somewhat
troublesome, also I ain't sure if ModuleTargetHelper really needs to
be placed into the top-level library regardless of its dependency on
non-public API. "promoting" test library class to the top-level
library comes w/ increased maintenance costs, the parent task[2]
explains that in more details.
[1] https://bugs.openjdk.java.net/browse/JDK-8211358
[2] https://bugs.openjdk.java.net/browse/JDK-8211290
-- Igor
On Oct 10, 2018, at 8:26 PM, Hamlin Li <huaming...@oracle.com
<mailto:huaming...@oracle.com>> wrote:
Hi Igor,
Would you please clarify your concern further? I mean why
ModuleTargetHelper can be put to top level when it use non-public
APIs e.g. jdk.internal.module.*?
Thank you
-Hamlin
On 2018/10/11 11:08 AM, Igor Ignatyev wrote:
Hi Hamlin,
as ModuleTargetHelper uses non-public API, I'd prefer not to have
in a common test library, and 8211976 suggests moving it closer to
tests. could you please explain why you decided to put it into the
library instead?
Thanks,
-- Igor
----- Original Message -----
From: huaming...@oracle.com <mailto:huaming...@oracle.com>
To: core-libs-dev@openjdk.java.net
<mailto:core-libs-dev@openjdk.java.net>
Sent: Wednesday, October 10, 2018 7:40:34 PM GMT -08:00 US/Canada
Pacific
Subject: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
Would you please review the following patch?
bug:
https://bugs.openjdk.java.net/browse/JDK-8186610
https://bugs.openjdk.java.net/browse/JDK-8211976
webrev: http://cr.openjdk.java.net/~mli/8186610/
<http://cr.openjdk.java.net/%7Emli/8186610/>
Thank you
-Hamlin
