[C++ PATCH] fix 3145

[Mark Mitchell]

--On Tuesday, November 20, 2001 08:10:26 PM +0000 Nathan Sidwell 
<nathan@codesourcery.com> wrote:

> Hi,
> this fixes 3145, one of the KDE virtual base bugs.
>
> The core of the problem is how build_vbase_path and get_base_distance
> behave. build_vbase_path was doing much more work than is now necessary,
> so I replaced it with a new build_base_path function, which copes
> with any kind of base binfo.  Then get_base_distance was called
> via convert_pointer_to_real in various circumstances that lead it
> to apparently locate the wrong base binfo.  I replaced get_base_distance
> with a new function lookup_base, which has (IMO) a cleaner interface.
> Following things through led to the elimination of convert_pointer_to
> and convert_pointer_to_real.

This is very exciting.  Getting rid of get_base_distance and
build_vbase_path is definitely nice.

I think that this patch is too big for the 3.0 branch; once it's approved
we'll put it on the mainline.

I'm having a hard time reading the patch due to the documentation being
a little sparse.  I think this is definitely the right approach, so I'll
point out some places where I think more comments would help, and then
we'll iterate.

! /* Build an access from EXPR to or from a base BINFO. CODE is
!    PLUS_EXPR if we're going from derived to base BINFO and MINUS_EXPR
!    if we're going from base BINFO to most derived. For that latter
!    case we cannot cross virtual bases. NONNULL is true if we know that
!    EXPR is not null.  EXPR may have pointer or class type and a
!    correctly qualifed pointer or class type is returned. */

  tree
! build_base_path (code, expr, binfo, nonnull)

I'm not sure I quite understand the interface.  Maybe this is the
right comment, or at least something close:

  /* Convert EXPR (an expression whose type is `A' or `A*'
     for some class type `A') to the base subobject `B' given by
     BINFO.  The expression returned will be of type `B' or `B*', matching
     the form provided in EXPR.  If CODE is PLUS_EXPR, `A' is derived
     from `B'; if it is MINUS_EXPR `B' is derived from `A'.  (In the latter
     case, there must be no virtual bases along the path from `A' to
     `B'.)  NONNULL is true if EXPR is known to be non-NULL.  */

And shouldn't NONNULL have type `bool'?

We should probably assert that CODE is either MINUS_EXPR or PLUS_EXPR
up front.

!          Under the 3.0 ABI, that offset is an entry in T's vtable.  */

Remove the first clause; there is now only one ABI.

+   bk_inaccessible = -3,   /* the base is inaccessible */

Capitalize the comments, I think?


! 	  /* If EXPR is NULL, then we don't need to do any arithmetic
! 	     to convert it:
!
! 	       [conv.ptr]
!
! 	       The null pointer value is converted to the null pointer
! 	       value of the destination type.  */
! 	  && !integer_zerop (expr))

Where did this check go in the new code?  Or is it no longer necessary?

This may have been an optimization so that:

  (A*)(B*)0

did not result in any code.  Would you check to see if we still generate
no instructions for such code?

+ static base_kind
+ lookup_base_recursive (binfo, base, access, within_current_scope,

Needs a comment.  And maybe some bool parameters.  And, I think the
pseudo-convention is to name these helper functions _r rather than
_recursive.

Thanks,


--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com