xfs
[Top] [All Lists]

Re: [PATCH 14/27] xfs: kill struct xfs_dir2_sf

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 14/27] xfs: kill struct xfs_dir2_sf
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 6 Jul 2011 10:05:34 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110706083345.GB19861@xxxxxxxxxxxxx>
References: <20110701094321.936534538@xxxxxxxxxxxxxxxxxxxxxx> <20110701094605.197942925@xxxxxxxxxxxxxxxxxxxxxx> <1309922658.3381.22.camel@doink> <20110706083345.GB19861@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-07-06 at 04:33 -0400, Christoph Hellwig wrote:
> On Tue, Jul 05, 2011 at 10:24:18PM -0500, Alex Elder wrote:
> > On Fri, 2011-07-01 at 05:43 -0400, Christoph Hellwig wrote:
> > > The list field of it is never cactually used, so all uses can simply be
> > > replaced with the xfs_dir2_sf_hdr_t type that it has as first member.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > 
> > Looks like a lot of places could be converted to use
> > "struct xfs_dir2_sf_hdr" rather than the typedef, but
> > it's not worth re-posting for that.  (Plus I suspect
> > such changes may be in forthcoming patches...)
> 
> In general they should, but I try to avoid that where it means
> massive formatting changes, as that just clutters up the patch.

Understood.

> > > + oldsfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
> > > +
> > >   ASSERT(dp->i_df.if_bytes == dp->i_d.di_size);
> > >   ASSERT(dp->i_df.if_u1.if_data != NULL);
> > 
> >     ASSERT(oldsfp != NULL);
> 
> What for?  We'll just dereference it later anyway.

It was simply because you already assigned oldsfp
the value you were asserting was null.  Your way
states something about the source value though,
so I guess it more directly states the condition
you're assuming here.

> > >  static xfs_ino_t
> > >  xfs_dir2_sf_get_ino(
> > > - struct xfs_dir2_sf      *sfp,
> > > + struct xfs_dir2_sf_hdr  *hdr,
> > 
> > I think I like the name "hdr" better than "sfp";
> > was it just too widespread a change to do a
> > similar rename elsewhere?  (xfs_dir2_block_to_sf()
> > uses "sfhp" already, though I like just "hdr".)
> 
> Yeah, I tried to keep the change small in general.  If people like it
> I can do a big sweep to convert stuff to struct types and common names
> as a follow-on.

No pressing need.  If you're inspired to do it, fine, but
it's readable despite the inconsistency.

                                        -Alex

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