Resolve numerous Coverity findings.#536
Conversation
| log.trace("Vertex {} has unknown lifecycle state", vertexId); | ||
| return null; | ||
| } else if (!tv.edges(Direction.BOTH).hasNext() && !tv.properties().hasNext()) { | ||
| if (!tv.edges(Direction.BOTH).hasNext() && !tv.properties().hasNext()) { |
There was a problem hiding this comment.
Why did you remove the log ?
And can you delete the dead code just above ?
There was a problem hiding this comment.
@davidclement90 This section is unreachable because it's conditioned on !foundVertexState and foundVertexState is set to !verifyVertexExistence (L93), which is always true since verifyVertexExistence is false (L48). I never would have found this if it wasn't for Coverity (and it took me awhile to see it even after it was flagged).
I just pushed updates to further remove the useless foundVertexState, which hopefully makes this clearer. Also removed the commented out code.
sjudeng
force-pushed
the
scan-fixes
branch
2 times, most recently
from
65cdbe3 to
ba2b9c2
Compare
| } else if (other instanceof JanusGraphElement) { | ||
| return ((JanusGraphElement) other).hasId() && getCompareId()==((JanusGraphElement)other).longId(); | ||
| } else if (other instanceof Element) { | ||
| } else if (other != null) { |
There was a problem hiding this comment.
Why this change from if (other instanceof Element) to if (other != null)?
The new check is always true, because the first line checks for other == null; however, the next line casts other to Element, so we really should have checked whether other instanceof Element is true.
Should this line be reverted to the original?
| File directory = new File(dir); | ||
| if (!directory.exists()) directory.mkdirs(); | ||
| if (!directory.exists() || !directory.isDirectory() || !directory.canWrite()) | ||
| if ((!directory.exists() && !directory.mkdirs()) || !directory.isDirectory() || !directory.canWrite()) |
There was a problem hiding this comment.
Since you're here, would you mind enclosing the if statement's single-line body in { ... } so that it's clear that it refers to a single statement, please? We've had other situations in this code base where indentation was counter to the actual syntax; would be nice to avoid that going forward.
Incidentally, I like how Go enforces braces for if statements, even for a single line, to avoid this ambiguity.
| File path = new File(dir); | ||
| if (!path.exists()) path.mkdirs(); | ||
| if (!path.exists() || !path.isDirectory() || !path.canWrite()) | ||
| if ((!path.exists() && !path.mkdirs()) || !path.isDirectory() || !path.canWrite()) |
There was a problem hiding this comment.
Please add {...} around if's statement body.
| @Override | ||
| public boolean hasNext() { | ||
| if (count != 0 && count % batchSize == 0 && batchSize * numBatches < limit) { | ||
| if (count != 0 && count % batchSize == 0 && count < limit) { |
There was a problem hiding this comment.
This change seems reasonable, but was this a Coverity-specified error or a separate change? I had to convince myself that this is correct, but this is a behavior change, so I'm curious how none of the unit tests failed before with this, and none required to be changed given this change (i.e., it should be possible to reproduce this error in a unittest such that it fails before, and succeeds after this change; or, that it succeeds before with an erroneous assertion, and this change requires changing said assertion to be correct).
Should this be a separate change?
There was a problem hiding this comment.
@mbrukman The Coverity finding was that batchSize*numBatches is performed at int32 but limit is int64. Upon review though I realized when count % batchSize == 0 (e.g. at the start of a new batch), count is equal to batchSize * numBatches. I was familiar with this line from reviewing when it was added (see #481 (comment)). @davidclement90 Does this seem okay?
Signed-off-by: sjudeng <sjudeng@users.noreply.github.com>
| @Override | ||
| public boolean hasNext() { | ||
| if (count != 0 && count % batchSize == 0 && batchSize * numBatches < limit) { | ||
| if (count != 0 && count % batchSize == 0 && count < limit) { |
Resolve numerous Coverity findings.
Resolve numerous Coverity findings.
4 participants
Relates to #535