lkml.org 
[lkml]   [2017]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 72/73] xfs: Convert mru cache to XArray
On Mon, 11 Dec 2017, Joe Perches wrote:

> > - I don't understand the error for xa_head here:
> >
> > struct xarray {
> > spinlock_t xa_lock;
> > gfp_t xa_flags;
> > void __rcu * xa_head;
> > };
> >
> > Do people really think that:
> >
> > struct xarray {
> > spinlock_t xa_lock;
> > gfp_t xa_flags;
> > void __rcu *xa_head;
> > };
> >
> > is more aesthetically pleasing? And not just that, but it's an *error*
> > so the former is *RIGHT* and this is *WRONG*. And not just a matter

Not sure what was meant here. Neither one is *WRONG* in the sense of
being invalid C code. In the sense of what checkpatch will accept, the
former is *WRONG* and the latter is *RIGHT* -- the opposite of what
was written.

> > of taste?
>
> No opinion really.
> That's from Andy Whitcroft's original implementation.

This one, at least, is easy to explain. The original version tends to
lead to bugs, or easily misunderstood code. Consider if another
variable was added to the declaration; it could easily turn into:

void __rcu * xa_head, xa_head2;

(The compiler will reject this, but it wouldn't if the underlying type
had been int instead of void.)

Doing it the other way makes the meaning a lot more clear:

void __rcu *xa_head, *xa_head2;

This is an idiom specifically intended to reduce the likelihood of
errors. Rather like avoiding assignments inside conditionals.

Alan Stern

\
 
 \ /
  Last update: 2017-12-12 16:52    [W:0.140 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site