Re: [PATCH] staging: lustre: move int literal to right side of comparison test

[Elizabeth Ferdman <gnudevliz@gmail.com>]

On Mon, Oct 03, 2016 at 09:04:56PM -0400, Oleg Drokin wrote:
> 
> On Oct 3, 2016, at 8:58 PM, Elizabeth Ferdman wrote:
> 
> > Move the integer literal 0 to the right side of the comparison test
> > for improved readability.
> > 
> > Error caught by checkpatch: 
> > Constants should be placed on the right side of the comparison.
> > 
> > Signed-off-by: Elizabeth Ferdman <gnudevliz@gmail.com>
> > ---
> > drivers/staging/lustre/lustre/lov/lov_object.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/lustre/lustre/lov/lov_object.c b/drivers/staging/lustre/lustre/lov/lov_object.c
> > index 52f7363..49a288f 100644
> > --- a/drivers/staging/lustre/lustre/lov/lov_object.c
> > +++ b/drivers/staging/lustre/lustre/lov/lov_object.c
> > @@ -597,7 +597,7 @@ static const struct lov_layout_operations lov_dispatch[] = {
> > 	enum lov_layout_type		    __llt;		  \
> > 									\
> > 	__llt = __obj->lo_type;					 \
> > -	LASSERT(0 <= __llt && __llt < ARRAY_SIZE(lov_dispatch));	\
> > +	LASSERT(__llt >= 0 && __llt < ARRAY_SIZE(lov_dispatch));	\
> 
> Do you think this is more readable than the version before it?
> 
>  at least the previous one easily shows that:
> 0 <= llt < ARRAY_SIZE(lov_dispatch)
> 

Hey Oleg,
In my opinion, yes. I like reading from left to right rather than
staring at a compound inequality so to me it's
more readable. __llt is the important part, so the way I think is
__llt has to be greater than or equal to 0 and less than
arraysize(lov_dispatch). Obviously the compound way says the exact same
thing, but it doesn't read from left to right in the same way. 

Actually, it looks so much like a compound inequality that
I initially glossed over the && part and kept thinking, 'ok the left
side results in either 0 or 1, and that's supposed to be less than array
size?'... which I realize doesn't make sense, but that's just the way I
interpreted it initially. Then I looked at it again and saw the &&.

Lastly, since the rule is to put constants on the right side, I figured
that way is more readable because that's the standard way of doing it,
therefore it's more readable...as in, it conforms to the way the rest of
the code in the kernel looks, it's not necessarily better or worse.

Thanks for the reply,
Liz

> (same with the rest).
> 
> > 	lov_dispatch[__llt].op(__VA_ARGS__);			    \
> > })
> > 
> > @@ -652,7 +652,7 @@ do {								    \
> > 									\
> > 	lov_conf_freeze(__obj);						\
> > 	__llt = __obj->lo_type;					 \
> > -	LASSERT(0 <= __llt && __llt < ARRAY_SIZE(lov_dispatch));	\
> > +	LASSERT(__llt >= 0 && __llt < ARRAY_SIZE(lov_dispatch));	\
> > 	lov_dispatch[__llt].op(__VA_ARGS__);			    \
> > 	lov_conf_thaw(__obj);						\
> > } while (0)
> > @@ -700,11 +700,11 @@ static int lov_layout_change(const struct lu_env *unused,
> > 	struct lu_env *env;
> > 	int refcheck;
> > 
> > -	LASSERT(0 <= lov->lo_type && lov->lo_type < ARRAY_SIZE(lov_dispatch));
> > +	LASSERT(lov->lo_type >= 0 && lov->lo_type < ARRAY_SIZE(lov_dispatch));
> > 
> > 	if (conf->u.coc_md)
> > 		llt = lov_type(conf->u.coc_md->lsm);
> > -	LASSERT(0 <= llt && llt < ARRAY_SIZE(lov_dispatch));
> > +	LASSERT(llt >= 0 && llt < ARRAY_SIZE(lov_dispatch));
> > 
> > 	cookie = cl_env_reenter();
> > 	env = cl_env_get(&refcheck);
> > -- 
> > 2.1.4
>