Code Review request for new "jsign" tool
mandy.chung at oracle.com
Fri Jul 1 10:22:23 PDT 2011
On 6/29/11 3:12 AM, Sean Mullan wrote:
> I have updated the webrev for the new jsign tool:
> 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.
>> L630-637: I believe if noExtract is true, destination will be null;
>> destination is non-null. Looks like you don't need the noExtract flag
>> 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 . 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.
More information about the jigsaw-dev