crunch-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Micah Whitacre <mkw...@gmail.com>
Subject Re: Another Coverity scan?
Date Wed, 20 May 2015 01:44:08 GMT
+1 to these changes.

What do people think about enabling the Maven checkstyle or findbugs
plugins as part of the standard build and possibly failing the build?  This
won't catch as much as Sean found but could eliminate some of the low
hanging fruit like unused imports, dead stores, missing hashCode, and maybe
more.

On Tue, May 19, 2015 at 4:54 PM, Sean Owen <sowen@cloudera.com> wrote:

> Hi all, I'd like to run by you all a little bit of analysis I did a while
> ago on Crunch (https://issues.apache.org/jira/browse/CRUNCH-380) and
> wanted
> to try again. I've been experimenting again with static analysis, in
> particular Coverity, and gave it another pass over the Crunch code.
>
> It turned up a few minor potential issues, and a number of reasonable
> touch-ups, so I wanted to propose another omnibus change to zap several of
> them.
>
> You can see the current state at
> https://scan.coverity.com/projects/1983?tab=overview but it does require
> login.
>
>
> As a preview, the minor touch-ups are:
>
> - Replace use of old junit.framework.* with org.unit.* for consistency
> - Remove some unused imports
> - String.getBytes() -> String.getBytes(Charset) to avoid platform
> dependence
> - Remove a few dead stores
> - Replace one Map.keySet() + many get()s with Map.entrySet() iteration
> - Remove a few @Nullable on method args that can't be (immediately
> dereferenced)
> - Closing some objects in a finally block that are Closable
> - Math.abs is technically a bad idea for something that can ==
> Integer.MIN_VALUE
>
>
> There are a few changes that might be minor bug fixes:
>
> Aggregators:1059
> "maxInputLength > 0 && next.length() > maxInputLength" also needs a check
> for next != null, but doesn't the second clause also need parentheses?
>
> TupleWritable:337
> The call to skip() doesn't check that the expected number of bytes were
> skipped.
>
> OrcWritable
> Missing hashCode for equals
>
> WritableGroupedTableType:97
> options is checked for null but is always dereferenced at the end
>
> CrunchOutputs:201
> baseContext can't be null at this point because of line 192
>
> SparkRuntime:342
> Not a bug but redundant I think since this occurs inside a block also
> guarded by "if (t instance MapReduceTarget) {"
>
>
> Does some of this sound worth a look at a patch? I've actually got it ready
> and can make a JIRA.
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message