staging: lustre: move int literal to right side of comparison test

PATCH In review Elizabeth Ferdman 2 participants 3 messages Oct 4, 2016 1 message not in the archive Staging

lore

Share & tools

lore

Elizabeth Ferdman

author

2016-10-04 00:58 UTC #0 lore raw

PATCH 1/1 staging: lustre: move int literal to right side of comparison test

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 <[email protected]>

---
 drivers/staging/lustre/lustre/lov/lov_object.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/staging/lustre/lustre/lov/lov_object.c +4 -4

index 52f7363..49a288f 100644
@@ -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));	\
 	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);

Signature · 1 line

2.1.4

Elizabeth Ferdman

author

2016-10-04 02:20 UTC #2 lore raw

In reply to a message not in the archive

Quoted 27 lines · Oleg Drokin

Quoted 16 lines · Elizabeth Ferdman
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 <[email protected]>
---
drivers/staging/lustre/lustre/lov/lov_object.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/lustre/lustre/lov/lov_object.c +0 -0
index 52f7363..49a288f 100644
drivers/staging/lustre/lustre/lov/lov_object.c static const struct lov_layout_operations lov_dispatch[] = { · line 600
Quoted 5 lines
drivers/staging/lustre/lustre/lov/lov_object.c +1 -1
@@ -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?

Quoted 3 lines

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

Quoted 31 lines

(same with the rest).

Quoted 28 lines

drivers/staging/lustre/lustre/lov/lov_object.c +3 -3

 	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);

Signature · 1 line

2.1.4

Julia Lawall

2016-10-04 06:00 UTC #3 lore raw

Quoted 46 lines · Elizabeth Ferdman

Quoted 27 lines · Oleg Drokin
Quoted 16 lines · Elizabeth Ferdman
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 <[email protected]>
---
drivers/staging/lustre/lustre/lov/lov_object.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/lustre/lustre/lov/lov_object.c +0 -0
index 52f7363..49a288f 100644
drivers/staging/lustre/lustre/lov/lov_object.c static const struct lov_layout_operations lov_dispatch[] = { · line 600
Quoted 5 lines
drivers/staging/lustre/lustre/lov/lov_object.c +1 -1
@@ -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?
Quoted 3 lines
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 &&.

Quoted 4 lines

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.

I think that A < x && x < B etc should be quite common in the kernel. The constant on the right rule is more appropriate, in my opinion, when there is only one comparison.

julia

Quoted 43 lines

Thanks for the reply,
Liz
Quoted 26 lines
(same with the rest).
Quoted 24 lines
drivers/staging/lustre/lustre/lov/lov_object.c +3 -3
 	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));
 
Quoted 5 lines
Quoted 4 lines
drivers/staging/lustre/lustre/lov/lov_object.c +0 -1
 	cookie = cl_env_reenter();
 	env = cl_env_get(&refcheck);
--
2.1.4
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161004022031.GA7732%40localhost.
For more options, visit https://groups.google.com/d/optout.