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

Groups > linux.kernel > #1366485 > unrolled thread

[PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function Dominique van den Broeck <domdevlin@free.fr> - 2016-03-29 19:20 +0200
    Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call  to a long function Joe Perches <joe@perches.com> - 2016-03-29 19:40 +0200
      Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call  to a long function Dominique van den Broeck <domdevlin@free.fr> - 2016-03-29 20:00 +0200
    Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to  a long function Peter Hurley <peter@hurleysoftware.com> - 2016-04-01 18:40 +0200
      Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call  to a long function Dominique van den Broeck <domdevlin@free.fr> - 2016-04-02 01:30 +0200
        Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to  a long function Peter Hurley <peter@hurleysoftware.com> - 2016-04-02 01:30 +0200

#1366485 — [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

Fixing a lone row exceeding 80 columns so the only remaining warnings
emitted by checkpatch.pl are missing comments on spinlocks and memory
barriers.

Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
 drivers/staging/fwserial/fwserial.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
index 4dd5304..c5f73ef 100644
--- a/drivers/staging/fwserial/fwserial.c
+++ b/drivers/staging/fwserial/fwserial.c
@@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
 
 	if (state == -1) {
 		set_bit(STOP_TX, &port->flags);
-		ret = wait_event_interruptible_timeout(port->wait_tx,
-					       !test_bit(IN_TX, &port->flags),
-					       10);
+		ret =
+		wait_event_interruptible_timeout(port->wait_tx,
+						 !test_bit(IN_TX, &port->flags),
+						 10);
+
 		if (ret == 0 || ret == -ERESTARTSYS) {
 			clear_bit(STOP_TX, &port->flags);
 			fwtty_restart_tx(port);
-- 
2.4.3

[toc] | [next] | [standalone]

#1366510 — Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

On Tue, 2016-03-29 at 19:14 +0200, Dominique van den Broeck wrote:
> Fixing a lone row exceeding 80 columns so the only remaining warnings
> emitted by checkpatch.pl are missing comments on spinlocks and memory
> barriers.
[]
> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
[]
> @@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
>  
>  	if (state == -1) {
>  		set_bit(STOP_TX, &port->flags);
> -		ret = wait_event_interruptible_timeout(port->wait_tx,
> -					       !test_bit(IN_TX, &port->flags),
> -					       10);
> +		ret =
> +		wait_event_interruptible_timeout(port->wait_tx,
> +						 !test_bit(IN_TX, &port->flags),
> +						 10);

Does this really look better to you?

Long identifiers like "wait_event_interruptible_timeout"
(32 chars) make
using 80 columns a bit silly.

Please remember checkpatch is a stupid script and that
not every warning it emits is dicta.

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

#1366519 — Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

> Does this really look better to you?
>
> Long identifiers like "wait_event_interruptible_timeout"
> (32 chars) make
> using 80 columns a bit silly.
> 
> Please remember checkpatch is a stupid script and that
> not every warning it emits is dicta.

Actually, not much and as a matter of fact, I hesitated before sending
this particular patch. That's also why I submitted it as the last one
of them all and why it fixes only a single row : so we can eventually
reject it easily.

I'm not a checkpatch.pl nazi neither and anyway, even the "Development
Process" document specify that it's not to be considered as a strict
rule if it makes the things worse.

But I found it useful anyway because this set brings back all the
warnings to only two categories (comments over spinlocks and memory
barriers) and also because this kind of minor corrections could still
be appreciated by people that have better things to do... :-)

Cheers.

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

#1369474 — Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

Hi Dominique,

On 03/29/2016 10:14 AM, Dominique van den Broeck wrote:
> Fixing a lone row exceeding 80 columns so the only remaining warnings
> emitted by checkpatch.pl are missing comments on spinlocks and memory
> barriers.
> 
> Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
> ---
>  drivers/staging/fwserial/fwserial.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/fwserial/fwserial.c b/drivers/staging/fwserial/fwserial.c
> index 4dd5304..c5f73ef 100644
> --- a/drivers/staging/fwserial/fwserial.c
> +++ b/drivers/staging/fwserial/fwserial.c
> @@ -1343,9 +1343,11 @@ static int fwtty_break_ctl(struct tty_struct *tty, int state)
>  
>  	if (state == -1) {
>  		set_bit(STOP_TX, &port->flags);
> -		ret = wait_event_interruptible_timeout(port->wait_tx,
> -					       !test_bit(IN_TX, &port->flags),
> -					       10);
> +		ret =
> +		wait_event_interruptible_timeout(port->wait_tx,
> +						 !test_bit(IN_TX, &port->flags),
> +						 10);
> +

I don't see a > 80-col line here?

And even if I did, this change would be super-ugly.
The preferred way to reduce this is to fold it into a helper function, like

	if (state == -1 && fwtty_wait_tx_complete(port))
		return -EINTR;


Regards,
Peter Hurley

>  		if (ret == 0 || ret == -ERESTARTSYS) {
>  			clear_bit(STOP_TX, &port->flags);
>  			fwtty_restart_tx(port);
> 

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

#1369654 — Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

Hello Peter,
Thanks a lot for your review and kind advice !

> I don't see a > 80-col line here?

In fact, it was not even a 80-col issue but a mis-aligned parenthesis
one. Realign the rows in this state would make them exceed the 80th
column.

I tend to agree with the fact that the way it currently is remains the
best one.

> And even if I did, this change would be super-ugly.
> The preferred way to reduce this is to fold it into a helper
> function

Actually, before I resend my patches, I have two or three small
questions:

1) My v1 patches already made it to staging and linux-next trees.
   Should I resend them anyway ?
2) Would it be helpful to people if I write a function the way you 
   specified it or would it be better to let it as is ?
3) If we don't, and then discard the last patch, shall I number « n/2 »
   or « n/3 » anyway ?

Forgive me if these questions are lame, I still have only a few
experience of the kernel tree. Documentation/SubmittingPatches states
that no one should be expected to refer to a previous set of patches,
so I suppose this would be « 1/2 » and « 2/2 » but I prefer being OK
about this from the beginning.

Thanks for caring.

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

#1369658 — Re: [PATCH 3/3] staging: fwserial: (coding style) Rewriting a call to a long function

On 04/01/2016 04:20 PM, Dominique van den Broeck wrote:
> Hello Peter,
> Thanks a lot for your review and kind advice !
> 
>> I don't see a > 80-col line here?
> 
> In fact, it was not even a 80-col issue but a mis-aligned parenthesis
> one. Realign the rows in this state would make them exceed the 80th
> column.

Ah, ok. Wasn't clear from the commit message.

> I tend to agree with the fact that the way it currently is remains the
> best one.

Ok.

>> And even if I did, this change would be super-ugly.
>> The preferred way to reduce this is to fold it into a helper
>> function
> 
> Actually, before I resend my patches, I have two or three small
> questions:
> 
> 1) My v1 patches already made it to staging and linux-next trees.
>    Should I resend them anyway ?

No, I didn't know they were already in staging-next.
Nevermind then :)

> 2) Would it be helpful to people if I write a function the way you 
>    specified it or would it be better to let it as is ?

As is, please.

> 3) If we don't, and then discard the last patch, shall I number « n/2 »
>    or « n/3 » anyway ?

n/a now.


> Forgive me if these questions are lame, I still have only a few
> experience of the kernel tree.

Your questions are not lame; no need to apologize.

> Documentation/SubmittingPatches states
> that no one should be expected to refer to a previous set of patches,
> so I suppose this would be « 1/2 » and « 2/2 » but I prefer being OK
> about this from the beginning.

If you would have sent the patches, yes, they would have been 1/2 and 2/2.
What I do there is send the v2 series in-reply-to the original 1/2 patch.


> Thanks for caring.

Regards,
Peter Hurley

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web