Code Review request for new "jsign" tool

[Mandy Chung]

  On 6/29/11 3:12 AM, Sean Mullan wrote:
> Mandy,
>
> I have updated the webrev for the new jsign tool:
>
> http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/jsign/webrev.01/
>
> This should address most of your comments (see below).
>
> Please let me know if it is ok to push this.
>

It looks good with one minor comment embedded below.
>
> For now, I have left the options as they are, but I agree we need to 
> define a more consistent option naming syntax across all of the new 
> jigsaw tools.

I'm fine with leaving the options as they are and revisit all new jigsaw 
tools' cli together.
> ...
>> ModuleFileFormat.java
>> L630-637: I believe if noExtract is true, destination will be null; 
>> otherwise,
>> destination is non-null. Looks like you don't need the noExtract flag 
>> and
>> perhaps replaced with needExtract method? Just a thought.
>
> I have not done this. Since there are several public methods that are 
> affected by this flag, I think it is better (and the code is easier to 
> understand) to use a flag to denote this behavior.

It's fine to keep the flag.  There are a couple places of the double 
negative check "if (!noExtract)"?  Would it be better to change the flag 
to "extract" rather than "noExtract"?

> I also fixed several unit test failures that were failing on Windows 
> due to the usage of FileChannel.map [1]. I replaced these with 
> FileChannel.read (and non-direct ByteBuffers). I did not see any 
> noticeable performance degradation, but we will probably need to do 
> some tests with larger module files to see if it makes any difference. 
> Also, these calls are only made when packaging the module files, and 
> not while reading.
>

Thanks for chasing this down.  Replacing FileChannel.map with 
FileChannel.read is a reasonable workaround.

Mandy