Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363168 > unrolled thread

[PATCH 4/5] mm/lru: is_file/active_lru can be boolean

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] mm/lru: is_file/active_lru can be boolean Yaowei Bai <baiyaowei@cmss.chinamobile.com> - 2016-03-23 03:30 +0100
    Re: [PATCH 4/5] mm/lru: is_file/active_lru can be boolean Joe Perches <joe@perches.com> - 2016-03-23 04:20 +0100
      Re: [PATCH 4/5] mm/lru: is_file/active_lru can be boolean Mel Gorman <mgorman@techsingularity.net> - 2016-03-23 12:50 +0100

#1363168 — [PATCH 4/5] mm/lru: is_file/active_lru can be boolean

This patch makes is_file/active_lru return bool to improve
readability due to these particular functions only using either
one or zero as their return value.

No functional change.

Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
---
 include/linux/mmzone.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 6de02ac3..652d60e 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -188,12 +188,12 @@ enum lru_list {
 
 #define for_each_evictable_lru(lru) for (lru = 0; lru <= LRU_ACTIVE_FILE; lru++)
 
-static inline int is_file_lru(enum lru_list lru)
+static inline bool is_file_lru(enum lru_list lru)
 {
 	return (lru == LRU_INACTIVE_FILE || lru == LRU_ACTIVE_FILE);
 }
 
-static inline int is_active_lru(enum lru_list lru)
+static inline bool is_active_lru(enum lru_list lru)
 {
 	return (lru == LRU_ACTIVE_ANON || lru == LRU_ACTIVE_FILE);
 }
-- 
1.9.1

[toc] | [next] | [standalone]

#1363186

On Wed, 2016-03-23 at 10:26 +0800, Yaowei Bai wrote:
> This patch makes is_file/active_lru return bool to improve
> readability due to these particular functions only using either
> one or zero as their return value.
> 
> No functional change.

These assignments to int should likely be modified too

$ git grep -w -n is_file_lru
include/linux/mmzone.h:191:static inline int is_file_lru(enum lru_list lru)
mm/vmscan.c:1404:                                   nr_taken, mode, is_file_lru(lru));
mm/vmscan.c:1525:                       int file = is_file_lru(lru);
mm/vmscan.c:1581:       int file = is_file_lru(lru);
mm/vmscan.c:1783:       int file = is_file_lru(lru);
mm/vmscan.c:1934:       if (is_file_lru(lru))
mm/vmscan.c:2129:                       int file = is_file_lru(lru);

[toc] | [prev] | [next] | [standalone]

#1363372

On Tue, Mar 22, 2016 at 08:17:08PM -0700, Joe Perches wrote:
> On Wed, 2016-03-23 at 10:26 +0800, Yaowei Bai wrote:
> > This patch makes is_file/active_lru return bool to improve
> > readability due to these particular functions only using either
> > one or zero as their return value.
> > 
> > No functional change.
> 
> These assignments to int should likely be modified too
> 

Which would lead to oddities as the ints are used as offsets within
enums. Patch 2 has a problem where a bool is then used as part of a
bitmask operation.

I stopped looking fairly early on. Conversions from int to bool as part
of a cleanup-only series are almost never useful and sometimes introduce
subtle breakage. It is only worth the conversion when the helper is being
modified for some other purpose.

On that grounds, NAK to the whole series as small problems were quickly
obvious and it's not worth the brainpower to find all the problems when
the end result does not give us a tangible improvement.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web