not to change anyone's mind one way or another about this … i agree with the aggressiveness of the extra checks, though i also agree with the annoying nature of the redundant/needless checks to quiet them.
but using this policy, i have found that a true false positive can be silenced by code that results in a false negative very easily if one is using some sort of accessor that could return null :
ITargetEnvironment env = getTargetEnvironmentService(); IDisassembler disassembler = (env != null ) ? env.getDisassembler() : null; if (disassembler == null) {
return;
}
// and then later
if (env.getLongestInstructionLength() ...)
the last line results in a warning, even though the logic above means the code will be exited if env is ever null.
but before i replaced this code, it contained the following calls, both of which actually could have potentially (though not likely) resulted in NPEs :
IDisassembler disassembler = getTargetEnvironmentService().getDisassembler(); if (disassembler == null) {
return;
}
// and then later
if (getTargetEnvironmentService().getLongestInstructionLength() ...)
my point is really that for this to be more useful and less annoying, the compiler not only has to be smarter about figuring out when a potential null may not occur, but also when function calls may return null and then be immediately dereferenced.
++ kirk
On 2010-Sep-2, at 5:36 AM, ext John Cortell wrote:
Fair enough. I randomly selected a CDT project yesterday (I don't
remember which one) and activated the warning. Most of the reported
instances where false positives and required needless/redundant checks to
quiet them. However, I've tried a few more projects this morning at home
and am seeing some good true positives, so I rescind my
recommendation.
At 07:07 PM 9/1/2010, Sergey Prigogin wrote:
-1. I haven't seen any false
positives, which required fixes that negatively affected code readability
or performance. In fact, I've seen very few false positives.
-sergey
On Wed, Sep 1, 2010 at 4:48 PM, Alena Laskavaia
<elaskavaia.cdt@xxxxxxxxx
> wrote:
- -1. The warning is very useful if it catch an error before runtime
and
- it does most of the time. And you can always write
- a code that won't have this warning, even if cost some redundant
- checks. How many actually warnings we are talking about here?
- You can always set it to ignore on some specific projects (such
as
- edc), but I won't do it for cdt.core, etc.
- On Wed, Sep 1, 2010 at 5:49 PM, John Cortell
<rat042@xxxxxxxxxxxxx>
wrote:
- > In
http://wiki.eclipse.org/CDT/policy, it's recommended that Potential
null
- > pointer access be set to 'warning'. The default is 'ignore'. I
think the
- > usefulness of this warning is too debatable to make it a
recommendation. The
- > compiler's ability to detect a possible null pointer use is too
weak and
- > enabling the warning results in many false positives,
requiring developers
- > to either add needless checks to the code, live with the
warnings, or
- > disable them at too large a scope (method). If no one objects,
I'll remove
- > that recommendation from the WIKI page.
- >
- > John
- >
- >
- > _______________________________________________
- > cdt-dev mailing list
- >
cdt-dev@xxxxxxxxxxx
- >
https://dev.eclipse.org/mailman/listinfo/cdt-dev
- >
- >
- _______________________________________________
- cdt-dev mailing list
- cdt-dev@xxxxxxxxxxx
-
https://dev.eclipse.org/mailman/listinfo/cdt-dev
_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev
|
