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

Groups > linux.kernel > #1342496 > unrolled thread

[PATCH trivial] include/linux/gfp.h: Improve the coding styles

Back to article view | Back to linux.kernel

Contents

  [PATCH trivial] include/linux/gfp.h: Improve the coding styles chengang@emindsoft.com.cn - 2016-02-24 23:30 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles SeongJae Park <sj38.park@gmail.com> - 2016-02-25 02:10 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:10 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Michal Hocko <mhocko@kernel.org> - 2016-02-25 10:00 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:30 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Michal Hocko <mhocko@kernel.org> - 2016-02-25 15:50 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:20 +0100
    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-25 10:30 +0100
      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 15:40 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-25 16:20 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:20 +0100
        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-25 17:10 +0100
          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-25 23:30 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-25 23:40 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:00 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles SeongJae Park <sj38.park@gmail.com> - 2016-02-26 00:20 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:10 +0100
            Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jianyu Zhan <nasa4836@gmail.com> - 2016-02-26 03:40 +0100
              Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-26 16:30 +0100
                Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-27 03:50 +0100
                  Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-27 15:30 +0100
                    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-27 18:00 +0100
                      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 01:20 +0100
                        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Mel Gorman <mgorman@techsingularity.net> - 2016-02-28 14:30 +0100
                          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 16:30 +0100
                    Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Jiri Kosina <jikos@kernel.org> - 2016-02-28 00:20 +0100
                      Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-28 01:50 +0100
                        Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Theodore Ts'o <tytso@mit.edu> - 2016-02-28 23:30 +0100
                          Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles Chen Gang <chengang@emindsoft.com.cn> - 2016-02-29 18:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1345023

On 2/27/16 10:45, Theodore Ts'o wrote:
> On Fri, Feb 26, 2016 at 11:26:02PM +0800, Chen Gang wrote:
>>> As for coding style, actually IMHO this patch is even _not_ a coding
>>> style, more like a code shuffle, indeed.
>>>
>>
>> "80 column limitation" is about coding style, I guess, all of us agree
>> with it.
> 
> No, it's been accepted that checkpatch requiring people to reformat
> code to within be 80 columns limitation was actively harmful, and it
> no longer does that.
> 
> Worse, it now complains when you split a printf string across lines,
> so there were patches that split a string across multiple lines to
> make checkpatch shut up.  And now there are patches that join the
> string back together.
> 
> And if you now start submitting patches to split them up again because
> you think the 80 column restriction is so darned important, that would
> be even ***more*** code churn.
> 

I don't think so. Of cause NOT the "CODE CHURN". It is not correct to
make an early decision during discussing.

"80 column limitation" is mentioned in "Documentation/CodingStyle", if
we have very good reason for it, we can break this limitation (for me,
what you said above are really some of good reasons).

But in our case (the patch), can anybody find any "good reasons" for it?
at least, at present, I can not find:

 - It is a common shared base header file, it is almost not used for
   code analyzing (e.g. git diff, git blame).

 - Is it helpful for "grep xxx filename | grep yyy"? Please check the
   patch, I can not find (maybe __GFP_MOVABL definition be? but it is
   still not obvious, if some member stick to, we can keep it no touch).

 - Could anyone find any good reasons for it within this patch?


> Which is one of the reasons why some of us aren't terribly happy with
> people who start running checkpatch -file on other people's code and
> start submitting patches, either through the trivial patch portal or
> not.
> 

For me, as a individual developer, I don't like this way, either. So of
cause, I don't care about this way.

I am just reading the common shared header files about mm. At least, I
can understand some common sense of mm, and also read through the whole
other headers to know what they are.

When I find something valuable more or less, I shall send related patch
for it.

> Mel, as an MM developer, has already NACK'ed the patch, which means
> you should not send the patch to **any** upstream maintainer for
> inclusion.

I don't think I "should not ...". I only care about correctness and
contribution, I don't care about any members ideas and their thinking.
When we have different ideas or thinking, we need discuss.

For common shared header files, for me, we should really take more care
about the coding styles.

 - If the common shared header files don't care about the coding styles,
   I guess any body files will have much more excuses for "do not care
   about coding styles".

 - That means our kernel whole source files need not care about coding
   styles at all!!

 - It is really really VERY BAD!!

If someone only dislike me to send the related patches, I suggest: Let
another member(s) "run checkpatch -file" on the whole "./include" sub-
directory, and fix all coding styles issues.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1345045

On Sat, Feb 27, 2016 at 10:32:04PM +0800, Chen Gang wrote:
> I don't think so. Of cause NOT the "CODE CHURN". It is not correct to
> make an early decision during discussing.

There is no discussion.  If the maintainer has NAK'ed it.  That's the
end of the dicsussion.  Period.  See:

ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html

Also note the comment from the above:

   NOTE: This means I'll only take whitespace/indentation fixes from the
   author or maintainer.

      	   	      	   			       	     - Ted

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

#1345123

On 2/28/16 00:53, Theodore Ts'o wrote:
> On Sat, Feb 27, 2016 at 10:32:04PM +0800, Chen Gang wrote:
>> I don't think so. Of cause NOT the "CODE CHURN". It is not correct to
>> make an early decision during discussing.
> 
> There is no discussion.  If the maintainer has NAK'ed it.  That's the
> end of the dicsussion.  Period.  See:
> 

For me, NAK also needs reasons.

And this issue is about "coding styles issue", I am not quite sure
whether trivial patch maintainer and mm maintainer are also the
maintainer for "coding styles issues".

I guess they are related with this patch, and their NAKs' reason are: mm
and trivial don't care about this coding style issue, is it correct?


> ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html
> 
> Also note the comment from the above:
> 
>    NOTE: This means I'll only take whitespace/indentation fixes from the
>    author or maintainer.

OK, thanks, it is really one proof for us. :-)

I guess, the file above almost means: except whitespace/indentation,
trivial patches don't consider about the coding styles issues. But can
we say coding styles issues are not issues in our kernel? (I guess not)

If we can not say, I guess one of your suggestion is useful (maybe be
as your suggestion): find one suitable member (I guess I am not), run
"checkpatch -file" under "./include", and fix all reported issues.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1345249

On Sun, Feb 28, 2016 at 08:21:40AM +0800, Chen Gang wrote:
> 
> On 2/28/16 00:53, Theodore Ts'o wrote:
> > On Sat, Feb 27, 2016 at 10:32:04PM +0800, Chen Gang wrote:
> >> I don't think so. Of cause NOT the "CODE CHURN". It is not correct to
> >> make an early decision during discussing.
> > 
> > There is no discussion.  If the maintainer has NAK'ed it.  That's the
> > end of the dicsussion.  Period.  See:
> > 
> 
> For me, NAK also needs reasons.
> 

You already got the reasons. Not only does a patch of this type interfere
with git blame which is important even in headers but I do not think the
patch actually improves the readability of the code. For example, the
comments move to the line after the defintions which to my eye at least
looks clumsy and weird.

> I guess they are related with this patch, and their NAKs' reason are: mm
> and trivial don't care about this coding style issue, is it correct?
> 

No. Coding style is important but it's a guideline not a law. There are
cases where breaking it results in perfectly readable code. At least one
my my own recent patches was flagged by checkpatch as having style issues
but fixing the style was considerably harder to read so I left it. If the
definitions in that header need to change again in the future and there
are style issues then they can be fixed in the context of a functional
change instead of patching style just for the sake of it.

-- 
Mel Gorman
SUSE Labs

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

#1345312

On 2/28/16 21:27, Mel Gorman wrote:
> On Sun, Feb 28, 2016 at 08:21:40AM +0800, Chen Gang wrote:
>>
>> For me, NAK also needs reasons.
>>
> 
> You already got the reasons. Not only does a patch of this type interfere
> with git blame which is important even in headers but I do not think the
> patch actually improves the readability of the code. For example, the
> comments move to the line after the defintions which to my eye at least
> looks clumsy and weird.
>

For me, in local headers, they may be often modified, and also may be
complex, so the code analyzing maybe also be used often. But in common
shared headers in ./include (e.g. gfp.h), most of them are simple enough.

 - Since common shared headers are usually simple, code analyzing is
   still useful, but not like the body files or local headers (code
   analyzing are very useful for body files and local headers).
 
 - Common shared headers are quite often read by most programmers, so
   common shared headers need take more care about its coding styles.

 - Then for common shared headers, the coding style is 1st.

And for __GFP_MOVABLE definition (with ZONE_MOVABLE), I guess, we can
keep it no touch (like what I originally said: if the related member
stick to, we can keep it no touch).

And for me, the other macro definitions which out of 80 columns, can be
fixed in normal ways (let the related comments ahead of macro definition
), does this change also have negative effect?


>> I guess they are related with this patch, and their NAKs' reason are: mm
>> and trivial don't care about this coding style issue, is it correct?
>>
> 
> No. Coding style is important but it's a guideline not a law.

Yes.

For me, vertical split window in vim is very useful, I almost always use
this feature when read source code in full screen under Macbook client,
when columns are 86+, it will be wrapped (I feel really not quite good).

And occasionally (really not often), we may copy/past part of contents
in the header files (e.g. constant definition) to the pdf file as
appendix.

So except the string broken, or "grep -rn xxx * | grep yyy", 80 columns
limitation is always helpful to me.

>                                                               There are
> cases where breaking it results in perfectly readable code. At least one
> my my own recent patches was flagged by checkpatch as having style issues
> but fixing the style was considerably harder to read so I left it. If the
> definitions in that header need to change again in the future and there
> are style issues then they can be fixed in the context of a functional
> change instead of patching style just for the sake of it.
> 

For me, except just modify the related contents, usually, we need devide
the patch into 2: one for real modification, the other for coding styles.

And in some of common, base, shared headers in ./include (e.g. gfp.h), I
guess, most of contents *should* not be changed quite often, so the bad
coding styles probably will be alive in a long term.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1345103

On Sat, 27 Feb 2016, Chen Gang wrote:

> > Mel, as an MM developer, has already NACK'ed the patch, which means
> > you should not send the patch to **any** upstream maintainer for
> > inclusion.
> 
> I don't think I "should not ...". I only care about correctness and
> contribution, I don't care about any members ideas and their thinking.
> When we have different ideas or thinking, we need discuss.

If by "discuss" you mean "30+ email thread about where to put a line 
break", please drop me from CC next time this discussion is going to 
happen. Thanks.

> For common shared header files, for me, we should really take more care
> about the coding styles.
> 
>  - If the common shared header files don't care about the coding styles,
>    I guess any body files will have much more excuses for "do not care
>    about coding styles".
> 
>  - That means our kernel whole source files need not care about coding
>    styles at all!!
> 
>  - It is really really VERY BAD!!
> 
> If someone only dislike me to send the related patches, I suggest: Let
> another member(s) "run checkpatch -file" on the whole "./include" sub-
> directory, and fix all coding styles issues.

Which is exactly what you shouldn't do.

The ultimate goal of the Linux kernel is not 100% strict complicance to 
the CodingStyle document no matter what. The ultimate goal is to have a 
kernel that is under control. By polluting git blame, you are taking on 
aspect of the "under control" away.

Common sense needs to be used; horribly terrible coding style needs to be 
fixed, sure. Is 82-characters long line horribly terrible coding style? 
No, it's not.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

#1345127

On 2/28/16 07:14, Jiri Kosina wrote:
> On Sat, 27 Feb 2016, Chen Gang wrote:
> 
>>> Mel, as an MM developer, has already NACK'ed the patch, which means
>>> you should not send the patch to **any** upstream maintainer for
>>> inclusion.
>>
>> I don't think I "should not ...". I only care about correctness and
>> contribution, I don't care about any members ideas and their thinking.
>> When we have different ideas or thinking, we need discuss.
> 
> If by "discuss" you mean "30+ email thread about where to put a line 
> break", please drop me from CC next time this discussion is going to 
> happen. Thanks.
> 

Excuse me, when I sent this patch, I did not know who I shall send to, I
have to reference to "./scripts/get_maintainer.pl".

If any members have no time to care about it (every members' time are
really expensive), please let me know (can reply directly).

Thanks.

>> For common shared header files, for me, we should really take more care
>> about the coding styles.
>>
>>  - If the common shared header files don't care about the coding styles,
>>    I guess any body files will have much more excuses for "do not care
>>    about coding styles".
>>
>>  - That means our kernel whole source files need not care about coding
>>    styles at all!!
>>
>>  - It is really really VERY BAD!!
>>
>> If someone only dislike me to send the related patches, I suggest: Let
>> another member(s) "run checkpatch -file" on the whole "./include" sub-
>> directory, and fix all coding styles issues.
> 
> Which is exactly what you shouldn't do.
> 

For me, I also guess, I am not the suitable member to do that (in fact,
I dislike to do like that - "run checkpath -file" on "./include").

> The ultimate goal of the Linux kernel is not 100% strict complicance to 
> the CodingStyle document no matter what. The ultimate goal is to have a 
> kernel that is under control. By polluting git blame, you are taking on 
> aspect of the "under control" away.
> 

Yes, the ultimate goal of CodingStyle is to have a kernel that is under
control.

For me, most of files in "./include" are common, simple, and shared
files, which are not quite related with code analyzing (e.g. git log -p,
or git blame), but they are read by others in most times. Is it correct?


> Common sense needs to be used; horribly terrible coding style needs to be 
> fixed, sure. Is 82-characters long line horribly terrible coding style? 
> No, it's not.
> 

For me, what you said above have effect on body files (in kernel, at
least, more than 95% source files are body files, I guess).

But in "./include", most of files are the interface inside and outside
of our kernel, we need take more care about their coding styles.

I often use vertical split window in vim in full screen mode to reading
source code, when I read c source files, I often split window vertically
for the related header files as reference.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

#1345413

On Sun, Feb 28, 2016 at 08:47:23AM +0800, Chen Gang wrote:
> 
> Excuse me, when I sent this patch, I did not know who I shall send to, I
> have to reference to "./scripts/get_maintainer.pl".
> 
> If any members have no time to care about it (every members' time are
> really expensive), please let me know (can reply directly).

Yes, everybody's time is very expensive.  So why are you wasting it
all with a "last post wins" style of argumentation?  A maintainer has
NAK'ed it.  Please drop this.

There is a reason why whitespace fixes are often consider to have
extreme negative value, and a deep suspicion that people are doing
this just to say that they have a patch in the kernel, perhaps in the
misapprehension that this will help them get a job.

Let me say that if I were a hiring manager, and I did a Google search
on a potential job application, and came across a thread like this, my
reaction would be extremely negative.

					- Ted

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

#1346032

On 2/29/16 06:23, Theodore Ts'o wrote:
> On Sun, Feb 28, 2016 at 08:47:23AM +0800, Chen Gang wrote:
>>
>> Excuse me, when I sent this patch, I did not know who I shall send to, I
>> have to reference to "./scripts/get_maintainer.pl".
>>
>> If any members have no time to care about it (every members' time are
>> really expensive), please let me know (can reply directly).
> 
> Yes, everybody's time is very expensive.  So why are you wasting it
> all with a "last post wins" style of argumentation?  A maintainer has
> NAK'ed it.  Please drop this.
> 

For me, I don't care about "last post wins".

But I care about the technical correctness, and for me, we (all of us)
need try our best to let the email reply as correct as we can, so can
avoid to mislead another readers.

So for me, if any members have new ideas, suggestions, or completions,
they can still reply at any time (may be next day, next week, or next
month ...).


> There is a reason why whitespace fixes are often consider to have
> extreme negative value, and a deep suspicion that people are doing
> this just to say that they have a patch in the kernel, perhaps in the
> misapprehension that this will help them get a job.
> 

For me, I don't think so, at least for me, contribution and learning is
my main goal in open source community, so I mainly focus on correctness.

All of us know when some related maintainer NAK'ed, the related patch,
of cause, must be dropped, the reason why I still reply the mail is: I
shall try to make the discussion/communication as correct as I can.

For "get a job", I guess, the open source community is helpful, but I
also suggest: if someone wants to "get a job", he/she should not depend
on the open source community (community has no duty for it).


> Let me say that if I were a hiring manager, and I did a Google search
> on a potential job application, and came across a thread like this, my
> reaction would be extremely negative.
> 

For me, job hunter and HR hunter can use open source community, but the
open source community's main goal is not for job hunter or HR hunter.  I
guess, the open source community's goal should be:

 - Develop the relate product (we can treat it as urgent thing, e.g.
   new features, bug fix).

 - Learning and discussing the product related technical issues (we can
   treat it as important thing, I guess, "coding styles issues" should
   be one of these issues).

Welcome any members ideas, suggestions, and completions for it.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web