xfs
[Top] [All Lists]

Re: [PATCH 1/3] xfs: cleanup bmap extent state macros

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 1/3] xfs: cleanup bmap extent state macros
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 14 Dec 2009 17:48:19 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1AB9A794DBDDF54A8A81BE2296F7BDFE012A6847@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <20091125000019.GA25432@xxxxxxxxxxxxx> <1AB9A794DBDDF54A8A81BE2296F7BDFE012A6847@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Mon, Dec 14, 2009 at 03:29:45PM -0600, Alex Elder wrote:
> Christoph Hellwig wrote:
> > Cleanup the extent state macros in the bmap code to use one common set of
> > flags that we can pass to the tracing code later and remove a lot of the
> > macro obsfucation.
> 
> This looks good.  Although this was motivated by making this code fit into
> the tracing model better, I prefer the way the result looks anyway.
> 
> Your changes are nice, straightforward mapping from the old to the
> new.  In a couple of spots it might be slightly clearer to assign
> a mask temporary, e.g. replace something like:
>          if ((state & (BMAP_LEFT_CONTIG | BMAP_LEFT_FILLING |
>                      BMAP_RIGHT_FILLING)) !=
>                     (BMAP_LEFT_CONTIG | BMAP_LEFT_FILLING |
>                      BMAP_RIGHT_FILLING))
> with
>          mask = BMAP_LEFT_CONTIG | BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING;
>          if (state & mask != mask)
> Nevertheless, I'm glad you didn't make changes like that because
> it was easier to review this way.  Something to consider for later.

Yeah, pondered it, but it didn't look totally clean with the mask
either.  It's a all bit borderline.

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