Hi Hamlin,
the patch looks good, just note to update Copyright dates.
-Felix Yang
________________________________
From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of
Hamlin Li <huaming...@oracle.com>
Sent: Friday, October 12, 2018 8:54:51 AM
To: Igor Ignatyev
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR of JDK-8186610,move ModuleUtils to top-level testlibrary
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
>>>>>
>>>>
>>>
>>
>
