Re: [PATCH]: PR29066 ptrmemfunc_vbit_in_delta is broken

[Mark Mitchell <mark at codesourcery dot com>]

Ryan Mansfield wrote:

> Is the following sufficient?

>>> QNX employees are cleared to contribute to binutils, gdb, and gcc.

Yes, that's great, thanks.

> Okay, I've made that change. Which g++.dg subdir should the new test
> case go in? I just picked 'other' because I wasn't sure.

Thanks for asking!  Please put it in g++.dg/expr.  Also, do you actually
need:

> +// { dg-options "" }

Normally, you only need that for a test case that isn't ISO-conforming,
and I would think it should certainly be possible to test for the
problem you're fixing here in an ISO-conforming manner.

> Thank you for all your help.

Actually, I'd imagine I'm driving you crazy; I keep coming up with new
nit-picks. :-)  Here are the last ones, I promise:


> +	      tree e1 = cp_build_binary_op (EQ_EXPR,
> +	  			            pfn0,
> +				      	    cp_convert (TREE_TYPE (pfn0),
> +						        integer_zero_node));

Use the low-level fold_convert here, rather than the C++ high-level
cp_convert routines.  We don't need to check for user-defined conversion
operators, etc...

(I'd suggest using fold_build2 instead of cp_build_binary_op, too --
except that we might be in a template, so we'd need a little care to
handle that.  The real truth is that in a template, we could make this
code faster; all we actually care about is the resulting type, and we
could short-circuit all of this.)

> +	      op0 = cp_build_binary_op (TRUTH_ANDIF_EXPR, e1, e2);
> +	      op1 = cp_convert (TREE_TYPE (op0), integer_one_node); 
> +	    }
> +     	  else 
> +	    {
> +	      op0 = build_ptrmemfunc_access_xpr (op0, pfn_identifier);

I don't think you compiled this code; this won't compile, since the
function is spelled "build_ptrmemfunc_access_expr".

> +	  if (TARGET_PTRMEMFUNC_VBIT_LOCATION
> +	      != ptrmemfunc_vbit_in_delta)
> +	    {
> +	      e2 = cp_build_binary_op (EQ_EXPR,
> +		      		       pfn0,
> +			   	       cp_convert (TREE_TYPE (pfn0),
> +						   integer_zero_node));

fold_convert here too.

And, I don't think the equality test here is correct.  For your case,
what you're implementing is:

  op0.delta == op1.delta && op0.pfn == op1.pfn

But, I think that's wrong for the NULL pointer case, because, again, the
high bits of delta don't matter.

What I think you need is:

 op0.pfn == op1.pfn
 && (/* They have the same delta or ...
     (op0.delta == op1.delta)
     /* ... both are NULL pointers.  */
     || (!op0.pfn && op0.delta & 1 == 0 && op1.delta & 1 == 0))

If you agree, you should update the comment above, explaining the code
that you are generating for this code.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713