crunch-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sean Owen <so...@cloudera.com>
Subject Re: Another Coverity scan?
Date Wed, 20 May 2015 09:03:18 GMT
I also have some mojo for findbugs and checkstyle from another project
that I can easily drop in, next. Findbugs is worth having as a target,
though the results are noisy (Coverity is better, and can do Findbugs
too, but is a little heavy to set up for regular usage and requires a
third party service). Checkstyle is decent to turn on though. I can
put in place a config that doesn't fail, which probably means
disabling most rules, but at least the structure is in place to catch
some stuff and turn on new rules when people want to do a clean up for
some style rule.

On Wed, May 20, 2015 at 9:57 AM, Gabriel Reid <gabriel.reid@gmail.com> wrote:
> +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
View raw message