Spotbugs remaining fixes #1303
Spotbugs remaining fixes #1303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1303 +/- ##
===============================================
- Coverage 67.644% 67.628% -0.015%
- Complexity 8188 8229 +41
===============================================
Files 560 562 +2
Lines 33462 33619 +157
Branches 5635 5640 +5
===============================================
+ Hits 22635 22736 +101
- Misses 8644 8709 +65
+ Partials 2183 2174 -9
|
| @@ -43,7 +43,7 @@ static int compress(final ByteBuffer in, final RansEncSymbol[][] syms, | |||
| // Deal with the remainder | |||
| l3 = 0xFF & in.get(in_size - 1); | |||
| for (i3 = in_size - 2; i3 > 4 * isz4 - 2 && i3 >= 0; i3--) { | |||
| final int c3 = 0xFF & in.get(i3 > -1 ? i3 : 0); | |||
| final int c3 = 0xFF & in.get(i3); | |||
There was a problem hiding this comment.
I thought this looked suspicious but I missed that the for loop directly restricts i3, e.g. i3 >= 0
| @@ -75,7 +75,7 @@ public String peek() { | |||
| private class TupleIterator extends AbstractIterator<Tuple<String, Long>> implements LocationAware { | |||
|
|
|||
| public TupleIterator() { | |||
| hasNext(); // Initialize the iterator, which appears to be a requirement of the parent class. TODO: Really? | |||
| super.hasNext(); // Initialize the iterator, which appears to be a requirement of the parent class. TODO: Really? | |||
There was a problem hiding this comment.
Why add super.? This class doesn't implement hasNext() and it's private.
There was a problem hiding this comment.
It's to make it clear that it's TupleIterator's superclass (AbstractIterator) that the hasNext() method resolves to, as opposed to hasNext() on the outer class, AsciiLineReaderIterator.
The default is the superclass, so this doesn't change the behaviour, it just makes it clear (and stops Spotbugs complaining). See https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#ia-potentially-ambiguous-invocation-of-either-an-inherited-or-outer-method-ia-ambiguous-invocation-of-inherited-or-outer-method
| if (content == null) { | ||
| writeNullContent(reference); | ||
| writeNullContent(); |
There was a problem hiding this comment.
This changes the behavior; before this would throw a NullPointerException. Is the new behavior the correct/desired behavior of this API?
There was a problem hiding this comment.
The NPE was introduced in ee10696#diff-d029b962e0444d3ad90c9c69372591bcL70. This fix removes the NPE, which seems like the right thing to do.
There was a problem hiding this comment.
It was introduced a month after the initial checkin of the code, which was 8 years ago. So it's worked this way for the lifetime of this code. If content == null is not a valid state, it would be better to remove this code rather than add a new feature.
There was a problem hiding this comment.
Fair enough. I've fixed this to explicitly throw a NPE in this case.
|
Rebased and merged this as #1331 |
Fix all the remaining issues found by Spotbugs (in addition to those in #1277 and #1278). In some cases it is not clear if there is a fix, or what the best fix would be that maintains existing behaviour, so I've added these cases to an exclude list. With the changes to the Gradle build file new violations will be picked up by causing the build to fail.
/cc @lbergelson
Checklist