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

Groups > linux.kernel > #1408580 > unrolled thread

[PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

Back to article view | Back to linux.kernel

Contents

  [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details chengang@emindsoft.com.cn - 2016-05-29 16:50 +0200
    Re: [PATCH trivial] include/linux/memblock.h: Clean up code for  several trivial details Joe Perches <joe@perches.com> - 2016-05-29 17:10 +0200
      Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several  trivial details Chen Gang <chengang@emindsoft.com.cn> - 2016-05-30 16:30 +0200
        Re: [PATCH trivial] include/linux/memblock.h: Clean up code for  several trivial details Joe Perches <joe@perches.com> - 2016-05-30 16:40 +0200
          Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several  trivial details Chen Gang <chengang@emindsoft.com.cn> - 2016-05-31 00:20 +0200
          Re: [PATCH trivial] include/linux/memblock.h: Clean up code for  several trivial details Chen Gang <chengang@emindsoft.com.cn> - 2016-05-31 04:10 +0200

#1408580 — [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

From: Chen Gang <chengang@emindsoft.com.cn>

Correct the function parameters alignment, since original code already
use both tabs and white spaces together for the incorrect parameters
alignment functions.

If one line can hold one statement within 80 columns, let it in one line
(original code did not consider about the tabs/spaces for 2nd line when
a statement is separated into 2 lines).

Use "!!" to let the boolean function return boolean value directly.

Try to let '\' aligned within one macro, since all related lines are
short enough.

Remove useless statement "idx = 0;", and always assign rgn within the
'for' statement.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/memblock.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 3106ac1..83d0410 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -73,8 +73,8 @@ extern bool movable_node_enabled;
 	if (memblock_debug) printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
 
 phys_addr_t memblock_find_in_range_node(phys_addr_t size, phys_addr_t align,
-					    phys_addr_t start, phys_addr_t end,
-					    int nid, ulong flags);
+					phys_addr_t start, phys_addr_t end,
+					int nid, ulong flags);
 phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
 				   phys_addr_t size, phys_addr_t align);
 phys_addr_t get_allocated_memblock_reserved_regions_info(phys_addr_t *addr);
@@ -110,7 +110,7 @@ void __next_mem_range_rev(u64 *idx, int nid, ulong flags,
 			  phys_addr_t *out_end, int *out_nid);
 
 void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start,
-			       phys_addr_t *out_end);
+				phys_addr_t *out_end);
 
 /**
  * for_each_mem_range - iterate through memblock areas from type_a and not
@@ -148,7 +148,7 @@ void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start,
 			       p_start, p_end, p_nid)			\
 	for (i = (u64)ULLONG_MAX,					\
 		     __next_mem_range_rev(&i, nid, flags, type_a, type_b,\
-					 p_start, p_end, p_nid);	\
+					  p_start, p_end, p_nid);	\
 	     i != (u64)ULLONG_MAX;					\
 	     __next_mem_range_rev(&i, nid, flags, type_a, type_b,	\
 				  p_start, p_end, p_nid))
@@ -163,8 +163,7 @@ void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start,
  * is initialized.
  */
 #define for_each_reserved_mem_region(i, p_start, p_end)			\
-	for (i = 0UL,							\
-	     __next_reserved_mem_region(&i, p_start, p_end);		\
+	for (i = 0UL, __next_reserved_mem_region(&i, p_start, p_end);	\
 	     i != (u64)ULLONG_MAX;					\
 	     __next_reserved_mem_region(&i, p_start, p_end))
 
@@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
 
 static inline bool memblock_is_mirror(struct memblock_region *m)
 {
-	return m->flags & MEMBLOCK_MIRROR;
+	return !!(m->flags & MEMBLOCK_MIRROR);
 }
 
 static inline bool memblock_is_nomap(struct memblock_region *m)
 {
-	return m->flags & MEMBLOCK_NOMAP;
+	return !!(m->flags & MEMBLOCK_NOMAP);
 }
 
 #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
@@ -403,15 +402,14 @@ static inline unsigned long memblock_region_reserved_end_pfn(const struct memblo
 }
 
 #define for_each_memblock(memblock_type, region)					\
-	for (region = memblock.memblock_type.regions;				\
+	for (region = memblock.memblock_type.regions;					\
 	     region < (memblock.memblock_type.regions + memblock.memblock_type.cnt);	\
 	     region++)
 
 #define for_each_memblock_type(memblock_type, rgn)			\
-	idx = 0;							\
-	rgn = &memblock_type->regions[idx];				\
-	for (idx = 0; idx < memblock_type->cnt;				\
-	     idx++,rgn = &memblock_type->regions[idx])
+	for (idx = 0, rgn = &memblock_type->regions[0];			\
+	     idx < memblock_type->cnt;					\
+	     idx++, rgn = &memblock_type->regions[idx])
 
 #ifdef CONFIG_MEMTEST
 extern void early_memtest(phys_addr_t start, phys_addr_t end);
-- 
1.9.3

[toc] | [next] | [standalone]

#1408585 — Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

On Sun, 2016-05-29 at 22:36 +0800, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> Correct the function parameters alignment, since original code already
> use both tabs and white spaces together for the incorrect parameters
> alignment functions.
> 
> If one line can hold one statement within 80 columns, let it in one line
> (original code did not consider about the tabs/spaces for 2nd line when
> a statement is separated into 2 lines).
> 
> Use "!!" to let the boolean function return boolean value directly.
[]
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
[]
> @@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
>  
>  static inline bool memblock_is_mirror(struct memblock_region *m)
>  {
> -	return m->flags & MEMBLOCK_MIRROR;
> +	return !!(m->flags & MEMBLOCK_MIRROR);

These !! uses are't necessary.
The compiler makes the bool return 0 or 1.

>  }
>  
>  static inline bool memblock_is_nomap(struct memblock_region *m)
>  {
> -	return m->flags & MEMBLOCK_NOMAP;
> +	return !!(m->flags & MEMBLOCK_NOMAP);
>  }

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

#1409164 — Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

On 5/29/16 23:08, Joe Perches wrote:
> On Sun, 2016-05-29 at 22:36 +0800, chengang@emindsoft.com.cn wrote:
>>
>> Use "!!" to let the boolean function return boolean value directly.
> []
>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> []
>> @@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
>>  
>>  static inline bool memblock_is_mirror(struct memblock_region *m)
>>  {
>> -	return m->flags & MEMBLOCK_MIRROR;
>> +	return !!(m->flags & MEMBLOCK_MIRROR);
> 
> These !! uses are't necessary.
> The compiler makes the bool return 0 or 1.
> 

No, they are not necessary. But for me, it will be more clearer, since
in our kernel (at least in include/linux/), almost all Boolean functions
use Boolean value or expression for return (and "!!" are often used).

Please help check, and welcome any additional ideas, suggestions, and
completions.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1409179 — Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote:
> On 5/29/16 23:08, Joe Perches wrote:
> > On Sun, 2016-05-29 at 22:36 +0800, chengang@emindsoft.com.cn wrote:
> > > Use "!!" to let the boolean function return boolean value directly.
> > []
> > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > []
> > > @@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
> > >  static inline bool memblock_is_mirror(struct memblock_region *m)
> > >  {
> > > -	return m->flags & MEMBLOCK_MIRROR;
> > > +	return !!(m->flags & MEMBLOCK_MIRROR);
> > These !! uses are't necessary.
> > The compiler makes the bool return 0 or 1.
> No, they are not necessary. But for me, it will be more clearer, since
> in our kernel (at least in include/linux/), almost all Boolean functions
> use Boolean value or expression for return (and "!!" are often used).

Opinions vary.

There seem to be fewer than 20 !! uses in bool return
functions in include/linux/

Finding the quantity of bool conversions in include/linux
from something other than 0, 1, true, or false to 0 or 1
is not trivial, but it is non-zero and seems rather more
than 20.

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

#1409701 — Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

On 5/30/16 22:33, Joe Perches wrote:
> On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote:
>> On 5/29/16 23:08, Joe Perches wrote:
>>> These !! uses are't necessary.
>>> The compiler makes the bool return 0 or 1.
>> No, they are not necessary. But for me, it will be more clearer, since
>> in our kernel (at least in include/linux/), almost all Boolean functions
>> use Boolean value or expression for return (and "!!" are often used).
> 
> Opinions vary.
>

OK, thanks. I shall send patch v2 for it within this week.
 
> There seem to be fewer than 20 !! uses in bool return
> functions in include/linux/
> 

[root@localhost linux]# pwd
/upstream/linux-next/include/linux
[root@localhost linux]# grep -rn '!!' * | grep return | wc -l
32
[root@localhost linux]# 

except 11 files which use int instead of bool as return value:

  ide.h:854
  mlx4/driver.h:76
  mlx5/device.h:744
  mmzone.h:793
  mtd/mtd.h:412
  nilfs2_fs.h:568
  nilfs2_fs.h:602
  nilfs2_fs.h:667
  nilfs2_fs.h:771
  page-flags.h:711
  pagemap.h:54
  
> Finding the quantity of bool conversions in include/linux
> from something other than 0, 1, true, or false to 0 or 1
> is not trivial, but it is non-zero and seems rather more
> than 20.
> 

[root@localhost linux]# grep -rn "\<return\>" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l
259

After give a glance, more than 60% are not for bool functions (so I
guess about 100 area hint).


Thanks
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1409778 — Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details

On 2016年05月30日 22:33, Joe Perches wrote:
> On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote:
>> No, they are not necessary. But for me, it will be more clearer, since
>> in our kernel (at least in include/linux/), almost all Boolean functions
>> use Boolean value or expression for return (and "!!" are often used).
> 
> Opinions vary.
> 
> There seem to be fewer than 20 !! uses in bool return
> functions in include/linux/
>

  ide.h:854
  mlx4/driver.h:76
  mlx5/device.h:744
  mmzone.h:793
  mtd/mtd.h:412
  nilfs2_fs.h:568
  nilfs2_fs.h:602
  nilfs2_fs.h:667
  nilfs2_fs.h:771
  page-flags.h:711
  pagemap.h:54

For me, we can change the related functions to Boolean functions
directly.
 
> Finding the quantity of bool conversions in include/linux
> from something other than 0, 1, true, or false to 0 or 1
> is not trivial, but it is non-zero and seems rather more
> than 20.
> 

[root@localhost linux]# grep -rn "\<return\>" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l
259

After give a glance, more than 60% are not for bool functions (so I
guess about 100 area hint).

But I guess, quite a few of none Boolean functions above can be changed
to Boolean functions, too (but have to read more code).

So I guess, about 200+ matches the hint (not use "!!" for & operation in
Boolean functions) in the 1000+ Boolean functions in include/linux.


All together, for me:

The return statement is much special than normal statements:

 - It is related with function's type (non-return function, Boolean
   function, or normal function), not only related with type cast within
   the statement itself.

 - Even for normal function, the type cast in return statement is also
   better: when reading source code, return statements have much more
   chances to be read than the function return type.

 - For finding Boolean function in existing normal functions, we often
   read the return value to know about whether it is a Boolean function
   or not.

So, I still suggest to add type cast explicitly in return statement.

Welcome any ideas, suggestions, and completions.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web