[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [platform-releng-dev] Progress with Platform Build and Sonar
|
On 07/11/2013 05:52 PM, Mickael Istria wrote:
Browsing the critical problems in JDT/Core I only found "false positives"
until now (read: "warnings which I don't want to see").
This highly depends on the definition of false-positive. A false-positive is not something you don't care, it's something that says
something wrong.
Being of that exact trade, I consciously put the term false positives in
quotes and gave my definition for this particular use of the term.
The biggest barrier against any new warning we implement in JDT is the
number of perceived false positives. Not annoying the user is an art,
which I don't see reflected in this (initial) sonar report.
It's not because you don't want to see the warning that it is not an issue.
Here is the list of CRITICAL issues for JDT/Core:
https://dev.eclipse.org/sonar/drilldown/violations/eclipse.jdt.core:eclipse.jdt.core?severity=CRITICAL
I see some things that are not false-positive, such as:
* Empty ifs that can consume CPU useless operations "if (blah) { /* comment */ }" should be more something like " /* if (blah) {
comment } */
Show me the JVM that wastes cycles on an empty block, the developers
of that JIT should be fired on the spot :)
No speculative optimizations, based on assumptions from 10 years ago, please!
For all the "critical" warnings that I saw, the condition still is
necessary because the other branch of the if is non-empty.
Some other issues marked as critical could indeed have there severity reduced, but are still overall Java bad practices:
* Code style "while (blah) { /*nothing*/ }" instead of a "do { something } while (still relevant)"
"Bad practice" is a totally subjective term. Some patterns are
successfully and consistently applied throughout our code base
with no problems caused, no danger involved, such that naming
them "bad practice" after the fact causes more confusion than
it helps. (The above example even makes a wrong suggestion).
* Arrays stored directly
JDT/Core has its very specific policies regarding arrays,
so all this is done entirely consciously, and must not be changed.
...
The goal here is not to hunt the false positive, but rather to track the real issue.
Looking at 100+ issues, I found one mildly relevant issue
(https://bugs.eclipse.org/412777).
That's a depressingly low ratio.
Also, as the platform build consists in many projects which have different concerns, I don't think it's possible to find a set of
rules that match all use cases (it's not the same way to develop SWT compared to developing PDE UI...)
So we need project specific rules.
So, to make this analysis useful we need tuning.
The goal is not to get it to 0 warnings.
Instead of tuning the tool, I think it's more efficient to read its report and decide which one of those reports deserve efforts.
Qualitative analysis over quantitative.
I tend to preach a very mild variant to this to JDT users.
But with no chance to get anywhere close to zero users will
simply walk away and not use the tool. Believe me.
- since I don't even know which tool (PMD? Checkstyle?) is giving
a particular warning, how would I suppress individual warnings?
So far it's using PMD and Checkstyle. I'll try to enable FindBugs soon.
So, what's the magic comment to silence a warning after deciding
that it is not relevant?
Seriously, without a better ratio of useful warnings vs. those that
lead nowhere, I don't believe this will be of any help.
If we have a chance to specify project specific rules and know
how to silence individual warnings (e.g., by special comments),
then the approach *could* perhaps be helpful, but I'm yet to be
convinced.
Honestly I think that Eclipse might be the wrong audience for these
tools. I could see that warnings of the kind I saw help in teams
with inexperienced developers and little investment on quality.
For those the style guides checked by the tools may be more helpful,
but for code of the quality of Eclipse and the seasoned developers
contributing to this code I believe that most of the warnings are
entirely beside the point.
The funniest warning I saw was against instances of
org.eclipse.jdt.core.dom.Statement, where the report advised me to
"Ensure that resources like this Statement object are closed after use".
That's not what I call a smart analysis. :)
We can do better.
cheers,
Stephan
