You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Stefan Bodewig <bo...@apache.org> on 2013/10/14 06:17:32 UTC
[CANCELLED][VOTE] Release Commons Compress 1.6
I think enough issues have been identified to warrant a second RC.
I'll take care of the bad version number in the release notes as well as
the PMD warnings in the ARJ-Package. ArjArchiveEntry's test coverage
has already been increased in trunk.
We should talk about the Clirr report further and come to a conclusion
what to do about the changes - if anything at all.
For the site's contents I don't see myself working on it before a second
RC. Help is more than welcome (not only with the site, of course).
Thanks to all who took the time to review the RC
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [compress] Dave's Code Review (was Re: [CANCELLED][VOTE] Release Commons Compress 1.6)
Posted by Stefan Bodewig <bo...@apache.org>.
On 2013-10-15, Stefan Bodewig wrote:
> But we probably should store the password as a char[] anyway
s/char/byte/
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[compress] Dave's Code Review (was Re: [CANCELLED][VOTE] Release Commons Compress 1.6)
Posted by Stefan Bodewig <bo...@apache.org>.
Hi
I'm going to address Dave's three mails in a single response
dam6923 . wrote:
> In SevenZFile.java
>
> Constructor...
> 1) Close file on exception instead of the current technique of keeping
> a "succeeded" flag.
This means I have to catch and rethrow the exception. I don't think I
like this better than the flag.
> 2) Move setting the password to the last line of the constructor so
> that it isn't stored if an exception is thrown at any point.
As the code stands this won't work as readHeaders needs the password
field. But we probably should store the password as a char[] anyway and
clear it when done - I'll change that before cutting the next release.
> General...
> 1) I see a few instances of
> if (file != null) {
> try {
> file.close();
> } catch (IOException ignored) { // NOPMD
> }
> file = null;
> }
> Can we snag the the IoUtils close methods in this project?
yep, but after the 1.6 release.
> 2) The class JavaDoc comments are wrapped after about 50 characters..
> extend to 80?
It may look wierd, but does it hurt?
> 3) There's a 'leaked resource' warning on line 190. A
> ByteArrayInputStream is not being closed, which makes sense as closing
> it does nothing, but for completeness sake perhaps or to protect
> against a time that the input-source could change and then it really
> is leaking.
> 4) Non-standard logging. There are many logging statements in the
> code... perhaps delegate that to a known logging framework or remove
> it all entirely?
> 5) Line 870 - magic number
will fix before the release (and remove the logging).
> org.apache.commons.compress.compressors.lzma.LZMACompressorInputStream#read()
>
> Current: count(ret == -1 ? -1 : 1);
> Change: count(ret == -1 ? 0 : 1);
doesn't make a difference if you look at count's implementation. Will
change it anyway.
> org.apache.commons.compress.archivers.ar.ArArchiveInputStream#getNextArEntry()
>
> The code to "skip to the next available entry" looks a bit inefficient
> because it reads one byte at a time in a loop. Consider using
> org.apache.commons.compress.utils.IOUtils.skip(InputStream, long)
>
> Consider using org.apache.commons.compress.utils.IOUtils.readFully(InputStream,
> byte[], int, int) on line 98 or else there may be an unnecessary
> exception thrown.
>
> In fact, everywhere there is a read for header information, consider
> using "readFully". Right now, lines 118-124 are not doing readFully
> and the return value is not being checked.
I agree we could improve this, but I'd prefer to keep the changes before
cutting the next RC down to a minimum. So "yes, but for 1.7".
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CANCELLED][VOTE] Release Commons Compress 1.6
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Oct 14, 2013 at 12:03 PM, dam6923 . <da...@gmail.com> wrote:
> Just a couple of immediate thoughts (just starting to look things over...)
>
> In SevenZFile.java
>
> Constructor...
> 1) Close file on exception instead of the current technique of keeping
> a "succeeded" flag.
> 2) Move setting the password to the last line of the constructor so
> that it isn't stored if an exception is thrown at any point.
>
> General...
> 1) I see a few instances of
> if (file != null) {
> try {
> file.close();
> } catch (IOException ignored) { // NOPMD
> }
> file = null;
> }
> Can we snag the the IoUtils close methods in this project?
>
> 2) The class JavaDoc comments are wrapped after about 50 characters..
> extend to 80?
120 is common in active [commons] projects these days.
Gary
> 3) There's a 'leaked resource' warning on line 190. A
> ByteArrayInputStream is not being closed, which makes sense as closing
> it does nothing, but for completeness sake perhaps or to protect
> against a time that the input-source could change and then it really
> is leaking.
> 4) Non-standard logging. There are many logging statements in the
> code... perhaps delegate that to a known logging framework or remove
> it all entirely?
> 5) Line 870 - magic number
>
> Thanks,
> David
>
> On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <bo...@apache.org> wrote:
>> I think enough issues have been identified to warrant a second RC.
>>
>> I'll take care of the bad version number in the release notes as well as
>> the PMD warnings in the ARJ-Package. ArjArchiveEntry's test coverage
>> has already been increased in trunk.
>>
>> We should talk about the Clirr report further and come to a conclusion
>> what to do about the changes - if anything at all.
>>
>> For the site's contents I don't see myself working on it before a second
>> RC. Help is more than welcome (not only with the site, of course).
>>
>> Thanks to all who took the time to review the RC
>>
>> Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
JUnit in Action, Second Edition
Spring Batch in Action
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CANCELLED][VOTE] Release Commons Compress 1.6
Posted by "dam6923 ." <da...@gmail.com>.
Just a couple of immediate thoughts (just starting to look things over...)
In SevenZFile.java
Constructor...
1) Close file on exception instead of the current technique of keeping
a "succeeded" flag.
2) Move setting the password to the last line of the constructor so
that it isn't stored if an exception is thrown at any point.
General...
1) I see a few instances of
if (file != null) {
try {
file.close();
} catch (IOException ignored) { // NOPMD
}
file = null;
}
Can we snag the the IoUtils close methods in this project?
2) The class JavaDoc comments are wrapped after about 50 characters..
extend to 80?
3) There's a 'leaked resource' warning on line 190. A
ByteArrayInputStream is not being closed, which makes sense as closing
it does nothing, but for completeness sake perhaps or to protect
against a time that the input-source could change and then it really
is leaking.
4) Non-standard logging. There are many logging statements in the
code... perhaps delegate that to a known logging framework or remove
it all entirely?
5) Line 870 - magic number
Thanks,
David
On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <bo...@apache.org> wrote:
> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package. ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC. Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CANCELLED][VOTE] Release Commons Compress 1.6
Posted by "dam6923 ." <da...@gmail.com>.
org.apache.commons.compress.compressors.lzma.LZMACompressorInputStream#read()
Current: count(ret == -1 ? -1 : 1);
Change: count(ret == -1 ? 0 : 1);
On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <bo...@apache.org> wrote:
> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package. ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC. Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [CANCELLED][VOTE] Release Commons Compress 1.6
Posted by "dam6923 ." <da...@gmail.com>.
org.apache.commons.compress.archivers.ar.ArArchiveInputStream#getNextArEntry()
The code to "skip to the next available entry" looks a bit inefficient
because it reads one byte at a time in a loop. Consider using
org.apache.commons.compress.utils.IOUtils.skip(InputStream, long)
Consider using org.apache.commons.compress.utils.IOUtils.readFully(InputStream,
byte[], int, int) on line 98 or else there may be an unnecessary
exception thrown.
In fact, everywhere there is a read for header information, consider
using "readFully". Right now, lines 118-124 are not doing readFully
and the return value is not being checked.
On Mon, Oct 14, 2013 at 12:17 AM, Stefan Bodewig <bo...@apache.org> wrote:
> I think enough issues have been identified to warrant a second RC.
>
> I'll take care of the bad version number in the release notes as well as
> the PMD warnings in the ARJ-Package. ArjArchiveEntry's test coverage
> has already been increased in trunk.
>
> We should talk about the Clirr report further and come to a conclusion
> what to do about the changes - if anything at all.
>
> For the site's contents I don't see myself working on it before a second
> RC. Help is more than welcome (not only with the site, of course).
>
> Thanks to all who took the time to review the RC
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org