Re: PR c++/24745: unpleasant warning for "if (NULL)"

["Manuel López-Ibáñez" <lopezibanez at gmail dot com>]

(sorry for the duplicate, both of us forgot to CC gcc-patches)

On 27/01/07, Jason Merrill <jason@redhat.com> wrote:
> Manuel López-Ibáñez wrote:
>
> First, please use diff -p for generating patches; I find context diffs
> easier to read than unified.  Unfortunately, this requires some svn
> tweaking to make it use an external diff program.

I don't get this. I used diff -up following contribution guidelines.
Are you looking to the correct patch?

http://gcc.gnu.org/ml/gcc-patches/2007-01/txt00108.txt

>  >  * typeck.c (build_binary_op): Fix logic for warning. Move warning to
>  > -Wpointer-arith.
>
> > -      && (/* If OP0 is NULL and OP1 is not a pointer, or vice versa.  */
> > -       (orig_op0 == null_node
> > -        && TREE_CODE (TREE_TYPE (op1)) != POINTER_TYPE)
> > -       /* Or vice versa.  */
> > -       || (orig_op1 == null_node
> > -           && TREE_CODE (TREE_TYPE (op0)) != POINTER_TYPE)
> > -       /* Or, both are NULL and the operation was not a comparison.  */
> > -       || (orig_op0 == null_node && orig_op1 == null_node
> > -           && code != EQ_EXPR && code != NE_EXPR)))
> > +       /* Or if one of OP0 or OP1 is not a pointer (null_node is not POINTER_TYPE).  */
> > +       || (!null_ptr_cst_p (orig_op0) && TREE_CODE (TREE_TYPE (op0)) != POINTER_TYPE)
> > +       || (!null_ptr_cst_p (orig_op1) && TREE_CODE (TREE_TYPE (op1)) != POINTER_TYPE)))
>
> This change seems like a small step backwards to me; both the old and
> new code should do the right thing, but the new code seems a bit slower
> and not as clear.  It seems to me that the only problem with the old
> code was:
>
> > -       /* Or, both are NULL and the operation was not a comparison.  */
> > -       || (orig_op0 == null_node && orig_op1 == null_node
> > -           && code != EQ_EXPR && code != NE_EXPR)))
> > +      && ( /* Both are NULL (or 0) and the operation was not a comparison.  */
> > +       (null_ptr_cst_p (orig_op0) && null_ptr_cst_p (orig_op1)
> > +        && code != EQ_EXPR && code != NE_EXPR)
>
> this section, which needs to use null_ptr_cst_p instead of checking only
> for null_node.

No, the old code doesn't check that one of the operands is actually a
null_node, so just adding null_ptr_cst_p as you suggest will produce
spurious warnings for things like 0 >= 0. It neither takes into
account that
TREE_CODE (TREE_TYPE (null_node) != POINTER_TYPE
is always true or that
null_ptr_cst_p (x) && TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE
is always false, so it broke things like __null == __null.

I am not sure how you reach the conclusion that this version is slower
than the previous one. We could save the value of null_ptr_cst_p
(orig_op0), but I hope the compiler is able to do it for me. I am not
sure how it can be further speed-up without breaking the testcases.

Cheers,

Manuel.