crunch-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gabriel Reid <gabriel.r...@gmail.com>
Subject Re: Another Coverity scan?
Date Wed, 20 May 2015 08:57:34 GMT
+1 to these changes, as well as findbugs (or similar) in the build.
Definitely can't hurt to have some extra automated checking on stuff like
this.
On Wed, May 20, 2015 at 02:45 Micah Whitacre <mkwhit@gmail.com> wrote:

> +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