Sonar analysis of OpenJDK 7 available (original) (raw)

Mario Torre neugens.limasoftware at gmail.com
Mon Nov 28 23:21:33 UTC 2011

Il giorno 28/nov/2011, alle ore 19:05, Kelly O'Hair ha scritto:

On Nov 24, 2011, at 12:36 AM, Roman Kennke wrote:

Hi Kelly,

Who gets to decide what the definition of "quality" here, or the configuration of what things to look for? I see 1,285 "violations" for using extra parens, Really? Things like return (true); are "violations"? return (true); is certainly correct code, but it's not good good style. Code quality is not only about correctness, but also (or most importantly) about maintainability. Things that makes difficult to read are violations. People find "return (true);" to be difficult to read? Really? I myself have no issue with extra parens, doesn't bother me in the least. But that's just my opinion. In a perfect world, and one where everyone agrees on what "good good style" is, you might, just might, consider changing hundreds or thousands of "return (true);" statements to "return true;", but how important is that in the face of everything else? In this case, you would think the risk would be low. But I would argue that there are many many more important quality issues than extra parens in code, and we should focus on the more important issues. When you include ALL quality issues like this one in a report, it creates a HUGE pile of issues that I consider an unfair representation of the code, and a daunting situation to deal with. My issue is one of priorities, we should focus on the more dangerous quality issues here, and not many style issues are high priority in my opinion.

I also find return (true) more difficult to read ;) (oh, well, I'm the guy who still asks people to stay within the 80 columns...) but I agree there are better priorities; I also find this kind of results a bit polluting, since they may actually create background noise that will make more important things less obvious.

However, all considered, those extremely low priority warnings maybe still good to have around, since they are easy task to be picked by students or people that want to contribute but actually don't have a full view of the platform; in other words they can lower significantly the entry barrier to contribution, which is a good thing imho.

I think the best solution is what has been suggested elsewhere in this thread, to have a set of defined rules, or perhaps even more than one set. I would be cool to create a list of "easy" tasks (like other projects like Gnome or LibreOffice have) so that people can start cleaning up the code.

It seems like a very nice tool, we just need to be careful what we change and why. I've trusted findbugs to do no harm when fixing what it reports, but I haven't found any other tool I would trust.

The tool PMD would tell you a variable was not used, but fail to detect that it's assignment used a method call that had critical side-effects. This tool seems to suffer from the same problem. So people need to be very very careful here. Critical side effects are bad bad quality IMO. Unnecessary side effects, I agree. But there are many APIs that rely on it, like buffers and I/O. So it has to be something a tool understands so that recommendations are not blindly followed. Please don't get me wrong, I think this Sonar tool is great, I just want to make sure we focus on the right things and don't create regressions by being too quick to change code that has been around for a long time. -kto

In the end, we should probably help the community to triage bugs, regressions and cleanups, most successful projects have such thing, and this tool (like other similar) may help in this.

I agree with you that the stability of the platform is much more important, but since we do have very careful reviews in place, we just need to fine tune what those tools tells us about the state of the code. In any case, common sense apply in my opinion, so yes, I completely support your point when you say that the tool should understand those shapes and that recommendations should be followed where it makes sense and not blindly.

Cheers, Mario

pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF

http://www.ladybug-studio.com

IcedRobot: www.icedrobot.org Proud GNU Classpath developer: http://www.classpath.org/ Read About us at: http://planet.classpath.org OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards: http://endsoftpatents.org/

More information about the discuss mailing list