Re: [cdt-dev] compiler warning policy

[<kirk.beitz@xxxxxxxxx>]

i don't really want to get into a discussion about whether or not arguably more readable code is the solution to a bug in the compiler's null-deref warning/error feature.

and i tried to emphasize i don't want to change the policy just because there's a bug in the compiler's null-deref warning/error feature.

my real point is that there is a bug in the compiler's null-deref warning/error feature, and i just would like to see more of a point made about getting it fixed as opposed to having to come up with such wonderfully creative suggestions.

++ kirk

On 2010-Sep-11, at 5:35 PM, ext Sergey Prigogin wrote:

Slightly longer but arguably more readable would be:

ITargetEnvironment env = getTargetEnvironmentService();

    if (env == null) {
return;
    }

    IDisassembler disassembler = env.getDisassembler();

    if (disassembler == null) {
return;
    }

    // and then later

    if (env.getLongestInstructionLength() ...)

I'm pretty sure that the compiler will be happy with this code.

-sergey

On Sat, Sep 11, 2010 at 5:23 PM, <kirk.beitz@xxxxxxxxx> wrote:

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

_______________________________________________
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

++ kirk beitz : nokia : austin + san diego : kirk.beitz@xxxxxxxxx ++