Re: [compress] Added in-memory support for zip and 7z

[M N <ma...@hotmail.com>]

Hi Stefan,



(...)

> read never indicates EOF as it stands, I think we should return -1
> rather than 0 when position equals size. WDYT?


Yes, indeed the contract specifies to return -1 in this case so we should change this.

>> In resize() method there is also a danger to overflow integer when we are close to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.

> True. One fix would be to ensure position + wanted is cut to
> Integer.MAX_VALUE in write (in case the sum turns negative) and in
> resize set len to newLength if newLength > Integer.MAX_VALUE / 2.


Allocation is an interesting topic in itself. I have looked at ArrayList allocation algorithm and I think they got it right.

First there is more conservative approach to allocation growth - 1.5 ratio instead of 2.0 as we use. Also there is an overflow handled.

Take a look at ArrayList.grow() method. The question is how to use this approach not to violate any license/intellectual property?


In the meantime I have created a patch with additional constructors, javadocs and also simple examples  for in-memory cases .

Patch is attached to Jira issue. Take a look, agree or disagree, thanks.


Cheers,

Maciej

________________________________
From: Stefan Bodewig <bo...@apache.org>
Sent: Saturday, October 22, 2016 4:46:34 PM
To: dev@commons.apache.org
Subject: Re: [compress] Added in-memory support for zip and 7z

Hi Maciej

patch applied.

On 2016-10-22, M N wrote:

> Going back to the fix - first I've done the homework and read the contract of SeekableByteChannel.position(long) method.

> It influences read() and  write() operation.

> Citation of the most important part:

> "Setting the position to a value that is greater than the current size is legal but does not change the size of the entity. A later attempt to read bytes at such a position will immediately return an end-of-file indication. A later attempt to write bytes at such a position will cause the entity to grow to accommodate the new bytes; the values of any bytes between the previous end-of-file and the newly-written bytes are unspecified."


> My changes to SeekableInMemoryByteChannel:

> 1. In read() method added check whether position exceeds size.

> 2. Added check for negative argument in position(long).

> 3. Added check for open channel in position(long) to fail immediately. In general the contract specifies to throw ClosedChannelExceptionexception by each method.

Thanks, I overlooked 1 and 3 and didn't really think about 2.

read never indicates EOF as it stands, I think we should return -1
rather than 0 when position equals size. WDYT?

> In resize() method there is also a danger to overflow integer when we are close to Integer.MAX_VALUE - either sum pos + wanted or shift may cause this.

True. One fix would be to ensure position + wanted is cut to
Integer.MAX_VALUE in write (in case the sum turns negative) and in
resize set len to newLength if newLength > Integer.MAX_VALUE / 2.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org

Re: [compress] Added in-memory support for zip and 7z

[Stefan Bodewig <bo...@apache.org>]

On 2016-11-01, M N wrote:

>> read never indicates EOF as it stands, I think we should return -1
>> rather than 0 when position equals size. WDYT?

> Yes, indeed the contract specifies to return -1 in this case so we
> should change this.

will do.

>>> In resize() method there is also a danger to overflow integer when
>>> we are close to Integer.MAX_VALUE - either sum pos + wanted or shift
>>> may cause this.

>> True. One fix would be to ensure position + wanted is cut to
>> Integer.MAX_VALUE in write (in case the sum turns negative) and in
>> resize set len to newLength if newLength > Integer.MAX_VALUE / 2.

> Allocation is an interesting topic in itself. I have looked at
> ArrayList allocation algorithm and I think they got it right.

> First there is more conservative approach to allocation growth - 1.5
> ratio instead of 2.0 as we use. Also there is an overflow handled.

> Take a look at ArrayList.grow() method. The question is how to use
> this approach not to violate any license/intellectual property?

Don't copy it. Really.

> In the meantime I have created a patch with additional constructors,
> javadocs and also simple examples for in-memory cases .

Many thanks, I've applied it.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org