Re: [PATCH] signal: clean up codestyle

Re: [PATCH] signal: clean up codestyle

[linmiaohe]

Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
> No functional change intended.

>Hey Miaohe,
>
>Thank you for the patch.
>I'm sure this is well-intended but afaict the whole file has more or less a consistent style already where e.g. sig-1 without spaces seems to be preferred. The same for the casts where most places use a single space.
>
>Now, I know CodingStyle.rst is on your side at least when it comes to the first point:
>
>Use one space around (on each side of) most binary and ternary operators, such as any of these::
>
>	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :
>
>but then you'd need to change each place in kernel/signal.c where that is currently not the case. Otherwise we end up with a weird mix.
>
>Thanks!
>Christian
> 

Many thanks for your reply. It looks it's better to make codestyle consistent. Will do it when I'am free. :)
Thanks again.

Re: [PATCH] signal: clean up codestyle

[linmiaohe]

Christian Brauner <christian.brauner@ubuntu.com> wrote:
>On Wed, Sep 02, 2020 at 01:34:59AM +0000, linmiaohe wrote:
>> Christian Brauner <christian.brauner@ubuntu.com> wrote:
>> >On Tue, Sep 01, 2020 at 06:39:05PM +0200, Oleg Nesterov wrote:
>> >> On 09/01, Christian Brauner wrote:
>> >Christian
>> 
>> Sorry for I did not get the imply.
>
>No need to apologize. That's my bad. 
>
>Maybe some context is useful.
>One of the reasons why we tend to sometimes not take changes such as this even though they would be covered by our officially documented coding style is to keep the churn minimal.
>Whenever functional change happens in codepaths such as this the risk of regressions is quite high. That's partially because we could use more tests to catch them (And if you're interested in stuff like this then writing selftests is always great. We can always use more of them.) but also simply because the code is complex. Having a lot of non-functional commits that don't really improve the legibility of the code significantly can become an issue for maintainers. Personally, I tend to be less worried about this but this is a collaborative endeavour. :)
>
>Thanks!
>Christian

I think I get the point this time. Many thanks for your detailed explaination. :)
Have a good day!

Re: [PATCH] signal: clean up codestyle

[Christian Brauner]

On Wed, Sep 02, 2020 at 01:34:59AM +0000, linmiaohe wrote:
> Christian Brauner <christian.brauner@ubuntu.com> wrote:
> >On Tue, Sep 01, 2020 at 06:39:05PM +0200, Oleg Nesterov wrote:
> >> On 09/01, Christian Brauner wrote:
> >> >
> >> > On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
> >> > > No functional change intended.
> >> >
> >> > Hey Miaohe,
> >> >
> >> > Thank you for the patch.
> >> > I'm sure this is well-intended but afaict the whole file has more or 
> >> > less a consistent style already where e.g. sig-1 without spaces 
> >> > seems to be preferred. The same for the casts where most places use 
> >> > a single space.
> >> >
> >> > Now, I know CodingStyle.rst is on your side at least when it comes 
> >> > to the first point:
> >> >
> >> > Use one space around (on each side of) most binary and ternary 
> >> > operators, such as any of these::
> >> >
> >> > 	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :
> >> >
> >> > but then you'd need to change each place in kernel/signal.c where 
> >> > that is currently not the case.
> >> 
> >> Or simply leave this code alone ;)
> >
> >I was trying to imply that by pointing out that this would be file-global change. I was likely too subtle. ;)
> >
> >Christian
> 
> Sorry for I did not get the imply.

No need to apologize. That's my bad. 

Maybe some context is useful.
One of the reasons why we tend to sometimes not take changes such as
this even though they would be covered by our officially documented
coding style is to keep the churn minimal.
Whenever functional change happens in codepaths such as this the risk of
regressions is quite high. That's partially because we could use more
tests to catch them (And if you're interested in stuff like this then
writing selftests is always great. We can always use more of them.) but
also simply because the code is complex. Having a lot of non-functional
commits that don't really improve the legibility of the code
significantly can become an issue for maintainers. Personally, I tend to
be less worried about this but this is a collaborative endeavour. :)

Thanks!
Christian

Re: [PATCH] signal: clean up codestyle

[linmiaohe]

Christian Brauner <christian.brauner@ubuntu.com> wrote:
>On Tue, Sep 01, 2020 at 06:39:05PM +0200, Oleg Nesterov wrote:
>> On 09/01, Christian Brauner wrote:
>> >
>> > On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
>> > > No functional change intended.
>> >
>> > Hey Miaohe,
>> >
>> > Thank you for the patch.
>> > I'm sure this is well-intended but afaict the whole file has more or 
>> > less a consistent style already where e.g. sig-1 without spaces 
>> > seems to be preferred. The same for the casts where most places use 
>> > a single space.
>> >
>> > Now, I know CodingStyle.rst is on your side at least when it comes 
>> > to the first point:
>> >
>> > Use one space around (on each side of) most binary and ternary 
>> > operators, such as any of these::
>> >
>> > 	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :
>> >
>> > but then you'd need to change each place in kernel/signal.c where 
>> > that is currently not the case.
>> 
>> Or simply leave this code alone ;)
>
>I was trying to imply that by pointing out that this would be file-global change. I was likely too subtle. ;)
>
>Christian

Sorry for I did not get the imply.

Re: [PATCH] signal: clean up codestyle

[Christian Brauner]

On Tue, Sep 01, 2020 at 06:39:05PM +0200, Oleg Nesterov wrote:
> On 09/01, Christian Brauner wrote:
> >
> > On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
> > > No functional change intended.
> >
> > Hey Miaohe,
> >
> > Thank you for the patch.
> > I'm sure this is well-intended but afaict the whole file has more or
> > less a consistent style already where e.g. sig-1 without spaces seems to
> > be preferred. The same for the casts where most places use a single
> > space.
> >
> > Now, I know CodingStyle.rst is on your side at least when it comes to
> > the first point:
> >
> > Use one space around (on each side of) most binary and ternary operators,
> > such as any of these::
> >
> > 	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :
> >
> > but then you'd need to change each place in kernel/signal.c where that
> > is currently not the case.
> 
> Or simply leave this code alone ;)

I was trying to imply that by pointing out that this would be
file-global change. I was likely too subtle. ;)

Christian

Re: [PATCH] signal: clean up codestyle

[Oleg Nesterov]

On 09/01, Christian Brauner wrote:
>
> On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
> > No functional change intended.
>
> Hey Miaohe,
>
> Thank you for the patch.
> I'm sure this is well-intended but afaict the whole file has more or
> less a consistent style already where e.g. sig-1 without spaces seems to
> be preferred. The same for the casts where most places use a single
> space.
>
> Now, I know CodingStyle.rst is on your side at least when it comes to
> the first point:
>
> Use one space around (on each side of) most binary and ternary operators,
> such as any of these::
>
> 	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :
>
> but then you'd need to change each place in kernel/signal.c where that
> is currently not the case.

Or simply leave this code alone ;)

To be honest I do not like the very idea of enforce-coding-style patches,
coding style is very personal and even this trivial (but imho pointless)
change can complicate the backporting of some bugfix. I hit this problem
quite often.

Oleg.

Re: [PATCH] signal: clean up codestyle

[Christian Brauner]

On Tue, Sep 01, 2020 at 07:58:00AM -0400, Miaohe Lin wrote:
> No functional change intended.

Hey Miaohe,

Thank you for the patch.
I'm sure this is well-intended but afaict the whole file has more or
less a consistent style already where e.g. sig-1 without spaces seems to
be preferred. The same for the casts where most places use a single
space.

Now, I know CodingStyle.rst is on your side at least when it comes to
the first point:

Use one space around (on each side of) most binary and ternary operators,
such as any of these::

	=  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :

but then you'd need to change each place in kernel/signal.c where that
is currently not the case. Otherwise we end up with a weird mix.

Thanks!
Christian

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  kernel/signal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index a38b3edc6851..10a31fafc35b 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1115,8 +1115,8 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
>  	q = __sigqueue_alloc(sig, t, GFP_ATOMIC, override_rlimit);
>  	if (q) {
>  		list_add_tail(&q->list, &pending->list);
> -		switch ((unsigned long) info) {
> -		case (unsigned long) SEND_SIG_NOINFO:
> +		switch ((unsigned long)info) {
> +		case (unsigned long)SEND_SIG_NOINFO:
>  			clear_siginfo(&q->info);
>  			q->info.si_signo = sig;
>  			q->info.si_errno = 0;
> @@ -1129,7 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
>  						 current_uid());
>  			rcu_read_unlock();
>  			break;
> -		case (unsigned long) SEND_SIG_PRIV:
> +		case (unsigned long)SEND_SIG_PRIV:
>  			clear_siginfo(&q->info);
>  			q->info.si_signo = sig;
>  			q->info.si_errno = 0;
> @@ -1314,7 +1314,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t)
>  	int sig = info->si_signo;
>  
>  	spin_lock_irqsave(&t->sighand->siglock, flags);
> -	action = &t->sighand->action[sig-1];
> +	action = &t->sighand->action[sig - 1];
>  	ignored = action->sa.sa_handler == SIG_IGN;
>  	blocked = sigismember(&t->blocked, sig);
>  	if (blocked || ignored) {
> -- 
> 2.19.1
> 

[PATCH] signal: clean up codestyle

[Miaohe Lin]

No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 kernel/signal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index a38b3edc6851..10a31fafc35b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1115,8 +1115,8 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
 	q = __sigqueue_alloc(sig, t, GFP_ATOMIC, override_rlimit);
 	if (q) {
 		list_add_tail(&q->list, &pending->list);
-		switch ((unsigned long) info) {
-		case (unsigned long) SEND_SIG_NOINFO:
+		switch ((unsigned long)info) {
+		case (unsigned long)SEND_SIG_NOINFO:
 			clear_siginfo(&q->info);
 			q->info.si_signo = sig;
 			q->info.si_errno = 0;
@@ -1129,7 +1129,7 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
 						 current_uid());
 			rcu_read_unlock();
 			break;
-		case (unsigned long) SEND_SIG_PRIV:
+		case (unsigned long)SEND_SIG_PRIV:
 			clear_siginfo(&q->info);
 			q->info.si_signo = sig;
 			q->info.si_errno = 0;
@@ -1314,7 +1314,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t)
 	int sig = info->si_signo;
 
 	spin_lock_irqsave(&t->sighand->siglock, flags);
-	action = &t->sighand->action[sig-1];
+	action = &t->sighand->action[sig - 1];
 	ignored = action->sa.sa_handler == SIG_IGN;
 	blocked = sigismember(&t->blocked, sig);
 	if (blocked || ignored) {
-- 
2.19.1