+1 Sean you can always keep cleaning this but the root cause is many, and issues like this will slowly creep in over time.
One of the root cause is that reviewing is hard in apache at the moment. I am really hoping we move to a set of tools that allows any contributor just to make changes -> send to review -> (get responses from jenkins) -> rework -> resend patch -> get approval from a committer -> submit. If you ask me to inspect a diff submitted on jira with absolutely no time available in day time. I will just look for general correctness. I would like to see static analysis output directly from jenkins, when someone checks in a patch. Robin ------ Robin Anil On Sat, Jun 9, 2012 at 6:17 PM, Sean Owen <[email protected]> wrote: > Guys, I'm preparing a large new patch that fixes style problems in the > code, for after the code freeze. This is my last pass at this for > Mahout. > > Style is not a big deal, though it's probably not good that random > non-standard Java is committed to the project. The only hard 'fix' for > this long-standing phenomenon is requiring a review process, and that > is too much. I don't think this project adheres to standards so much, > and such is life. > > However, simply turning on code inspections in a modern IDE like > IntelilJ is turning up plain bugs in the code. I want to call out a > few, because I want to fix them (after 0.7), but also because I want > to make the point that static analysis can find bugs. Because it can, > it should. I think open source projects can and should be the finest > output of the best and brightest. And at "mere" Google, stuff that > static analysis finds would never have gotten to even code review. > Hence I am somewhat dismayed to see so many problems being committed > without review into the code base. > > Here's a taste... > > > TestMeanShift.testCanopyEuclideanMRJob(), line 365 > > Vector[] permutedRaw = new Vector[raw.length]; > for (int i = 0; i < raw.length; i++) > permutedRaw = raw; > > Oops. I think this accidentally accomplishes its goal of a copy, but > it intends a deep copy and that's not what it does. > > > > LocalSSVDPCADenseTest.runSSVDSolver(), line 107: > > xi.assign(Functions.mult(1 / m)); > > m is an integer, so 1/m == 0 for any m > 1. This is not the intent, > but the test is actually crafted to work only when xi is assigned to > 0. > > > Let's raise the bar please. >
