xfs
[Top] [All Lists]

Re: [PATCH 17/27] xfs: kill struct xfs_dir2_block

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 17/27] xfs: kill struct xfs_dir2_block
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 6 Jul 2011 10:11:50 -0500
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, <xfs@xxxxxxxxxxx>
In-reply-to: <20110706083714.GE19861@xxxxxxxxxxxxx>
References: <20110701094321.936534538@xxxxxxxxxxxxxxxxxxxxxx> <20110701094605.827598983@xxxxxxxxxxxxxxxxxxxxxx> <20110706023157.GL1026@dastard> <20110706083714.GE19861@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-07-06 at 04:37 -0400, Christoph Hellwig wrote:
> On Wed, Jul 06, 2011 at 12:31:57PM +1000, Dave Chinner wrote:
> > >   btp = xfs_dir2_block_tail_p(mp, hdr);
> > > - ptr = (char *)block->u;
> > > + ptr = (char *)(hdr + 1);
> > >   endptr = (char *)xfs_dir2_block_leaf_p(btp);
> > 
> > That is slightly less obvious what it is doing. It's jumping over
> > the entire header, but could easily be confused with jumping one
> > byte in.

I actually have become pretty accustomed to this
idiom and have a (small) preference for the way it
is here rather than using a new macro that does
the same thing.  Either way is fine with me though.

                                        -Alex

> > Perhaps adding a wrapper e.g. xfs_dir2_block_data_p(hdr) to match
> > the xfs_dir2_block_tail_p() and xfs_dir2_block_leaf_p() wrappers,
> > and converting all the other cases to use this as well?
> 
> I had that in the initial version, but given that we usually use
> the result as char, and not one of the two types of the union just
> made the code very messy.
> 
> I can try it again, maybe as a add-on patch at the end so that we can
> decide if it actually improves anything.
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs



<Prev in Thread] Current Thread [Next in Thread>