linux-kernel - Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 10 Jun 2016 08:26:00 +0800
From:	Chen Gang <chengang@...ndsoft.com.cn>
To:	Rik van Riel <riel@...hat.com>, akpm@...ux-foundation.org,
	trivial@...nel.org
CC:	vdavydov@...tuozzo.com, hannes@...xchg.org, mhocko@...e.cz,
	davem@...emloft.net, tj@...nel.org, linux-kernel@...r.kernel.org,
	Chen Gang <gang.chen.5i5j@...il.com>
Subject: Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only

On 6/10/16 01:39, Rik van Riel wrote:
> On Thu, 2016-06-09 at 23:23 +0800, chengang@...ndsoft.com.cn wrote:
>> From: Chen Gang <chengang@...ndsoft.com.cn>
>>
>> Merge several statements to one return statement, since the new
>> return
>> statement is still simple enough.
> 
> This code is not simple, and any change that
> makes it harder to read needs a good reason.
> 
> At least in my opinion, your return statement
> merging thing makes the code harder to read.
>

For me, every member has his/her own taste, we need talk about it case
by case.

[...]

>>  
>>  static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>>  {
>> -	if (mem_cgroup_disabled())
>> -		return 0;
>> -
>> -	return memcg->css.id;
>> +	return mem_cgroup_disabled() ? 0 : memcg->css.id;
>>  }
>>  

For me, this is the simplest way for using "? :", so it is easy enough (
I guess, Linux kernel does not completely reject "? :").

>>  /**
>> @@ -341,10 +338,7 @@ static inline unsigned short
>> mem_cgroup_id(struct mem_cgroup *memcg)
>>   */
>>  static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short
>> id)
>>  {
>> -	struct cgroup_subsys_state *css;
>> -
>> -	css = css_from_id(id, &memory_cgrp_subsys);
>> -	return mem_cgroup_from_css(css);
>> +	return mem_cgroup_from_css(css_from_id(id, &memory_cgrp_subsys));
>>  }
>>  

For me, this case may depend on various members' tastes (although it is
simple enough to me -- it is in one line within 80 columns, and related
with 2 functions' call).

For this case, if any of another member suggests to keep original code
no touch, too, I shall send patch v2 for it (keep it no touch).

>>  /**
>> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>>  
>>  static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>>  {
>> -	if (mem_cgroup_disabled())
>> -		return true;
>> -	return !!(memcg->css.flags & CSS_ONLINE);
>> +	return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
>>  }
>>  

For me, this is almost the simplest way for using "||", we can find
this case in include/linux (60+ cases for '||', and 150+ for '&&'), and
the new return statement is in one line within 80 columns.

The new return statement is not the simplest, but it is still simple
enough (which should be better than original several lines statements).

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.