>> On Oct 19, 2016, at 5:05 PM, Steve Drach <steve.dr...@oracle.com> wrote:
>>
>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>>>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/
>>>
>>> Issue a warning is good in the case public classes are added in a concealed
>>> package. Some comments:
>>>
>>> Main.java
>>>
>>> addExtendedModuleAttributes does not seem to be the appropriate method to
>>> initialize concealedPackages. It would be better to set concealedPackages
>>> in a separate method that will be called for each modular JAR.
>>
>> I made a simple change to existing code, I wasn’t intending to make
>> significant changes to jar tool. Of course as time goes on, jar tool
>> continues to grow into a bigger hair ball. It would definitely benefit from
>> major cosmetic surgery. Perhaps I don’t understand the point you are trying
>> to make.
>
> It made it hard for review and future maintainability. It was not obvious
> to me when I reviewed the code whether this misses any case.
>
> Refactoring it is a small change.
Just to be clear, you’d like me to take lines 1988-1995 and put them in a
separate method that gets called by addExtendedModuleAttributes?
>
>>
>>>
>>> ConcealedPackage.java test
>>>
>>> 60 Path destination = userdir.resolve("classes”);
>>>
>>> I suggest to use Paths.get(“classes”) rather than ${user.dir}.
>>
>> Is there a performance advantage or some other reason for doing this? The
>> way I do it seems reasonable.
>
> I just want to point out that jtreg will do the clean up if you use the
> scratch directory.
>
>>
>>> jtreg will clean up the JTwork/scratch directory after the test run.
>>
>> That’s what the docs say but I’ve seen problems in the past with windows
>> machines, so I just got in the habit
>> of doing my own clean up.
>>
>
> Up to you.
>
>>>
>>> 63 // add an empty directory
>>> 64
>>> Files.createDirectory(destination.resolve("p").resolve("internal"));
>>>
>>> I suggest to take this out. The test verifies if "jar tf mmr.jar” succeeds.
>>
>> Ok. Just trying to make it exactly the same as the jar structure in the bug
>> report.
>
> I updated the JBS issue to clarify that per the recent discussion.
>
>>
>>>
>>> 92 private int jar(String cmd) {
>>> Nit: this can simply take a varargs and no need to split:
>>> jar(String… options)
>>
>> I like the String because it’s more readable and I suspect the split isn’t
>> that costly.
>>
>
> I just point out that it’s a kind of silly to concat all args and then split
> it.
>
> Mandy
>
