linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Chen Gang <chengang@emindsoft.com.cn>
To: Michal Hocko <mhocko@kernel.org>
Cc: akpm@linux-foundation.org, trivial@kernel.org,
	vdavydov@virtuozzo.com, hannes@cmpxchg.org, davem@davemloft.net,
	tj@kernel.org, riel@redhat.com, linux-kernel@vger.kernel.org,
	Chen Gang <gang.chen.5i5j@gmail.com>
Subject: Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
Date: Fri, 10 Jun 2016 08:40:30 +0800	[thread overview]
Message-ID: <575A0C7E.9020500@emindsoft.com.cn> (raw)
In-Reply-To: <20160609154605.GH24777@dhcp22.suse.cz>


On 6/9/16 23:46, Michal Hocko wrote:
> On Thu 09-06-16 23:23:52, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>>
>> Merge several statements to one return statement, since the new return
>> statement is still simple enough.
>>
>> Try to let the second line function parameters almost align with the
>> first line parameter (try to be within 80 columns, and in one line).
>>
>> The comments can fully use 80 columns which can save one line.
>>
>> Use parameter name newpage instead of new (which will be key word color
>> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate()
>> already uses newpage.
> 
> What is the point of these changes? It removes few lines but does that
> actually make the code easier to read? To be honest I am not a big fan
> of such a stylist changes unless they are in a series of other changes
> which actually tweak the functionality. This just brings more churn
> to the git history.
> 

For me, if one line is simple enough, we need not use several lines to
express the same thing, especially a line within 80 columns can express
the whole function work in headers.

The public header files do not like body files, they are simple enough,
but their contents are often read through by another readers. So we need
more careful for coding styles, but less careful for git history.

> That's being said, I appreciate an interest in making the code cleaner
> but try to think whether these changes are actually helpful and who is
> going to benefit from them.
> 

For me, another readers can get benefit more or less from it: if read a
simple line can know the whole thing (function work), why need we read
multiple lines?

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

  reply	other threads:[~2016-06-10  0:35 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-09 15:23 [PATCH trivial] include/linux/memcontrol.h: Clean up code only chengang
2016-06-09 15:46 ` Michal Hocko
2016-06-10  0:40   ` Chen Gang [this message]
2016-06-10  6:14     ` Michal Hocko
2016-06-10  8:34       ` Chen Gang
2016-06-09 17:39 ` Rik van Riel
2016-06-10  0:26   ` Chen Gang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=575A0C7E.9020500@emindsoft.com.cn \
    --to=chengang@emindsoft.com.cn \
    --cc=akpm@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=gang.chen.5i5j@gmail.com \
    --cc=hannes@cmpxchg.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@kernel.org \
    --cc=riel@redhat.com \
    --cc=tj@kernel.org \
    --cc=trivial@kernel.org \
    --cc=vdavydov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).