> On Oct 19, 2016, at 5:05 PM, Steve Drach <[email protected]> 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.
>
>>
>> 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
