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  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.

Powered by blists - more mailing lists