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

Groups > linux.kernel > #1568018 > unrolled thread

[PATCH] greybus: Fix coding stye error

Back to article view | Back to linux.kernel

Contents

  [PATCH] greybus: Fix coding stye error mrossibellom@gmail.com - 2017-01-27 11:20 +0100
    Re: [PATCH] greybus: Fix coding stye error Greg KH <gregkh@linuxfoundation.org> - 2017-01-27 11:30 +0100
    Re: [PATCH] greybus: Fix coding stye error Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-27 13:40 +0100
      Re: [PATCH] greybus: Fix coding stye error Johan Hovold <johan@kernel.org> - 2017-01-27 14:20 +0100
        Re: [PATCH] greybus: Fix coding stye error Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-27 14:30 +0100
          Re: [PATCH] greybus: Fix coding stye error Johan Hovold <johan@kernel.org> - 2017-01-27 15:10 +0100
            Re: [PATCH] greybus: Fix coding stye error Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-27 15:20 +0100
              Re: [PATCH] greybus: Fix coding stye error Maxime Rossi Bellom <mrossibellom@gmail.com> - 2017-01-27 15:40 +0100
                Re: [PATCH] greybus: Fix coding stye error Maxime Rossi Bellom <mrossibellom@gmail.com> - 2017-01-27 15:50 +0100
                Re: [PATCH] greybus: Fix coding stye error Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-27 15:50 +0100
      Re: [PATCH] greybus: Fix coding stye error Johan Hovold <johan@kernel.org> - 2017-01-27 14:40 +0100
        Re: [PATCH] greybus: Fix coding stye error Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-27 15:20 +0100

#1568018 — [PATCH] greybus: Fix coding stye error

From: Maxime Rossi Bellom <mrossibellom@gmail.com>

Split line before funtion argument.

Error was reported by checkpatch.pl as
WARNING: line over 80 characters

Signed-off-by: Maxime Rossi Bellom <mrossibellom@gmail.com>
---
 drivers/staging/greybus/fw-management.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
index 3cd6cf0..d88117e 100644
--- a/drivers/staging/greybus/fw-management.c
+++ b/drivers/staging/greybus/fw-management.c
@@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
 		 */
 		fw_mgmt->mode_switch_started = true;
 
-		ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
+		ret = gb_interface_request_mode_switch(
+				fw_mgmt->connection->intf);
 		if (ret) {
 			dev_err(fw_mgmt->parent, "Mode-switch failed: %d\n",
 				ret);
-- 
2.7.4

[toc] | [next] | [standalone]

#1568029

On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
> From: Maxime Rossi Bellom <mrossibellom@gmail.com>
> 
> Split line before funtion argument.
> 
> Error was reported by checkpatch.pl as
> WARNING: line over 80 characters
> 
> Signed-off-by: Maxime Rossi Bellom <mrossibellom@gmail.com>
> ---
>  drivers/staging/greybus/fw-management.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 3cd6cf0..d88117e 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
>  		 */
>  		fw_mgmt->mode_switch_started = true;
>  
> -		ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
> +		ret = gb_interface_request_mode_switch(
> +				fw_mgmt->connection->intf);

That's horrid looking, please don't make code look worse, this is fine
as-is.

thanks,

greg k-h

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

#1568329

On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
> From: Maxime Rossi Bellom <mrossibellom@gmail.com>
> 
> Split line before funtion argument.
> 
> Error was reported by checkpatch.pl as
> WARNING: line over 80 characters
> 
> Signed-off-by: Maxime Rossi Bellom <mrossibellom@gmail.com>
> ---
>  drivers/staging/greybus/fw-management.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 3cd6cf0..d88117e 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
>  		 */
>  		fw_mgmt->mode_switch_started = true;
>  
> -		ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
> +		ret = gb_interface_request_mode_switch(
> +				fw_mgmt->connection->intf);


Another option would be to change the name from
gb_interface_request_mode_switch() to gb_get_mode_switch() or something
along those lines.  It requires understanding the code to say what a
good name is.

regards,
dan carpenter

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

#1568345

On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
> The name sucks is what I'm saying.  Please fix it eventually.

I disagree. The name uses a common prefix that reflects the object it is
working on. This should not be changed. As it is currently named, the
function is also self-documenting. Trying to abbreviate the name just to
meet a 80 col limit (when there are alternatives) is just silly.

Johan

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

#1568351

On Fri, Jan 27, 2017 at 02:08:06PM +0100, Johan Hovold wrote:
> On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
> > The name sucks is what I'm saying.  Please fix it eventually.
> 
> I disagree. The name uses a common prefix that reflects the object it is
> working on. This should not be changed. As it is currently named, the
> function is also self-documenting. Trying to abbreviate the name just to
> meet a 80 col limit (when there are alternatives) is just silly.

gb_interface_request_mode_switch()

It's not self documenting because there are no verbs in that sentence.
I guess in JAVA it would be considered good style.

This is "Enterprise Quality" in the derogatory sense of the term.  The
prefix makes everything too long and doesn't add any value.  We don't
keep the 80 character limit because of small screens, we keep it to
discourage this type of code.

regards,
dan carpenter

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

#1568391

On Fri, Jan 27, 2017 at 04:24:47PM +0300, Dan Carpenter wrote:
> On Fri, Jan 27, 2017 at 02:08:06PM +0100, Johan Hovold wrote:
> > On Fri, Jan 27, 2017 at 03:47:21PM +0300, Dan Carpenter wrote:
> > > The name sucks is what I'm saying.  Please fix it eventually.
> > 
> > I disagree. The name uses a common prefix that reflects the object it is
> > working on. This should not be changed. As it is currently named, the
> > function is also self-documenting. Trying to abbreviate the name just to
> > meet a 80 col limit (when there are alternatives) is just silly.
> 
> gb_interface_request_mode_switch()
> 
> It's not self documenting because there are no verbs in that sentence.

"request" is a verb.

We're requesting that the remote interface initiates a mode switch
(something which it can refuse).

It's a function that is only a called a couple of times. No need to
rewrite it as gb_intf_rms() or such (even if gb_intf would be a
reasonable prefix).

You comments about missing verbs etc make me wonder how you parse the
name:

	gb_interface		- a common prefix, indicating object
				  being acted on
	request_mode_switch 	- describes what is done

we're not fetching anything as your "gb_get_mode_switch()" suggestion
indicates you read it as.

> I guess in JAVA it would be considered good style.
> 
> This is "Enterprise Quality" in the derogatory sense of the term.  The
> prefix makes everything too long and doesn't add any value.  We don't
> keep the 80 character limit because of small screens, we keep it to
> discourage this type of code.

And to keep indentation down.

The kernel already has similar function names such as
"usb_driver_set_configuration" or "pm_runtime_mark_last_busy" which
follow the same pattern (prefix - verb - object).

Johan

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

#1568393

I guess I'm not a fan of really long prefixes...  I've complained before
to other people so don't feel special.  Prefixes are something that
people add without thinking as if they always make the code better and
I do blame JAVA influence for this...

The "_interface" really doesn't add anything because that's always the
first parameter that we pass.  It's just filler.

I think "try" is better than "request" probably.  Request is regularly
used as a noun and try never is.

But I don't care at this point.

regards,
dan carpenter

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

#1568404

On Fri, Jan 27, 2017 at 05:13:01PM +0300, Dan Carpenter wrote:
> I guess I'm not a fan of really long prefixes...  I've complained before
> to other people so don't feel special.  Prefixes are something that
> people add without thinking as if they always make the code better and
> I do blame JAVA influence for this...
> 
> The "_interface" really doesn't add anything because that's always the
> first parameter that we pass.  It's just filler.
> 
> I think "try" is better than "request" probably.  Request is regularly
> used as a noun and try never is.
> 
> But I don't care at this point.
> 
> regards,
> dan carpenter
>

I don't feel confortable removing a word in the function name as I don't 
fully understand the code.

What about shorting interface into intf as it is used in connection->intf ?

Johan, what do you mean by using connection->intf in this case ? Are you
talking about a global variable usable in this context ?

Thanks,
Maxime

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

#1568407

On Fri, Jan 27, 2017 at 05:43:47PM +0300, Dan Carpenter wrote:
> Heh.  I would just leave this one alone for now.
> 
> Everyone hates all the potential changes more than we hate going over
> 80 characters.
> 
> regards,
> dan carpenter
>

Understood.

Thanks
Maxime

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

#1568409

Heh.  I would just leave this one alone for now.

Everyone hates all the potential changes more than we hate going over
80 characters.

regards,
dan carpenter

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

#1568364

On Fri, Jan 27, 2017 at 03:29:30PM +0300, Dan Carpenter wrote:
> On Fri, Jan 27, 2017 at 11:11:21AM +0100, mrossibellom@gmail.com wrote:
> > From: Maxime Rossi Bellom <mrossibellom@gmail.com>
> > 
> > Split line before funtion argument.
> > 
> > Error was reported by checkpatch.pl as
> > WARNING: line over 80 characters
> > 
> > Signed-off-by: Maxime Rossi Bellom <mrossibellom@gmail.com>
> > ---
> >  drivers/staging/greybus/fw-management.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> > index 3cd6cf0..d88117e 100644
> > --- a/drivers/staging/greybus/fw-management.c
> > +++ b/drivers/staging/greybus/fw-management.c
> > @@ -516,7 +516,8 @@ static int fw_mgmt_ioctl(struct fw_mgmt *fw_mgmt, unsigned int cmd,
> >  		 */
> >  		fw_mgmt->mode_switch_started = true;
> >  
> > -		ret = gb_interface_request_mode_switch(fw_mgmt->connection->intf);
> > +		ret = gb_interface_request_mode_switch(
> > +				fw_mgmt->connection->intf);
> 
> 
> Another option would be to change the name from
> gb_interface_request_mode_switch() to gb_get_mode_switch() or something
> along those lines.  It requires understanding the code to say what a
> good name is.

Indeed, and please do not rename this function.

Adding a temporary variable for the interface pointer would be a better
way to deal with this if anyone is really bothered by this long line.

In fact, it may even be possible to just use connection->intf here in
this case.

But I'd just leave it as is.

Thanks,
Johan

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

#1568392

The name sucks is what I'm saying.  Please fix it eventually.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web