Patchwork [net-next,05/16] i40e: fix long lines

login
register
mail settings
Submitter Jeff Kirsher
Date Jan. 10, 2014, 8:58 a.m.
Message ID <1389344336-1558-6-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/309191/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Jan. 10, 2014, 8:58 a.m.
From: Mitch Williams <mitch.a.williams@intel.com>

Avoid over-length lines in order to appease checkpatch.

Change-ID: I63820a710acf798f49d2f85c610228711af84f72
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c    | 3 ++-
 drivers/net/ethernet/intel/i40e/i40e_prototype.h | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)
Joe Perches - Jan. 10, 2014, 9:07 a.m.
On Fri, 2014-01-10 at 00:58 -0800, Jeff Kirsher wrote:
> From: Mitch Williams <mitch.a.williams@intel.com>
> 
> Avoid over-length lines in order to appease checkpatch.

I think you can and should ignore long line warnings
where appropriate.

and...

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
[]
> @@ -681,7 +681,8 @@ aq_add_vsi_exit:
>   * @cmd_details: pointer to command details structure or NULL
>   **/
>  i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> -				u16 seid, bool set, struct i40e_asq_cmd_details *cmd_details)
> +				u16 seid, bool set,
> +				struct i40e_asq_cmd_details *cmd_details)

Another way this could be changed is to put the
return value on a separate line like:

i40e_status
i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool set,
				    struct i40e_asq_cmd_details *cmd_details)

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
[]
> @@ -93,9 +93,9 @@ i40e_status i40e_aq_set_vsi_broadcast(struct i40e_hw *hw,
>  				u16 vsi_id, bool set_filter,
>  				struct i40e_asq_cmd_details *cmd_details);
>  i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> -				u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
> +		u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);

i40e_status
i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 vsi_id, bool set,
				    struct i40e_asq_cmd_details *cmd_details);

etc...

but once you use extremely long 35+ characters
function names, 80 columns gets a bit silly too.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Jan. 10, 2014, 10:02 a.m.
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> []
> > @@ -681,7 +681,8 @@ aq_add_vsi_exit:
> >   * @cmd_details: pointer to command details structure or NULL
> >   **/
> >  i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> > -				u16 seid, bool set, struct i40e_asq_cmd_details *cmd_details)
> > +				u16 seid, bool set,
> > +				struct i40e_asq_cmd_details *cmd_details)
> 
> Another way this could be changed is to put the
> return value on a separate line like:
> 
> i40e_status
> i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool set,
> 				    struct i40e_asq_cmd_details *cmd_details)

Personally I prefer that so I can find the definition by grepping
for '^function_name' - but it doesn't seem to be done in any linux
source files.

> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> []
> > @@ -93,9 +93,9 @@ i40e_status i40e_aq_set_vsi_broadcast(struct i40e_hw *hw,
> >  				u16 vsi_id, bool set_filter,
> >  				struct i40e_asq_cmd_details *cmd_details);
> >  i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> > -				u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
> > +		u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
> 
> i40e_status
> i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 vsi_id, bool set,
> 				    struct i40e_asq_cmd_details *cmd_details);
> 
> etc...
> 
> but once you use extremely long 35+ characters
> function names, 80 columns gets a bit silly too.

Again this only a real problem when the coding stand requires that
continuation lines have the parameters lined up under the '('.
If they are indented by 4 spaces (as many of the BSDs do) then you
do stand a chance of laying out a call in a reasonable number of
vertical lines.

My Actual preferred layout is to exclude tabs, indent by 4 spaces
and double-indent continuation lines.
I'll also start continuation lines (of expressions) with an operator
to make it even clearer they are continuation lines.
When I'm scan-reading code I tend to concentrate on the LHS of each line.
- so that is where the information need to be.

Not that I expect Linux to change!
(Other code bases have much worse layout rules.)

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 10, 2014, 10:35 a.m.
On Fri, 2014-01-10 at 10:02 +0000, David Laight wrote:
> > Another way this could be changed is to put the
> > return value on a separate line like:
> > 
> > i40e_status
> > i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool set,
> > 				    struct i40e_asq_cmd_details *cmd_details)
> 
> Personally I prefer that so I can find the definition by grepping
> for '^function_name' - but it doesn't seem to be done in any linux
> source files.

That form is used in a lot of files.
$ git grep -E "^[a-z_]+\("


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Williams, Mitch A - Jan. 10, 2014, 6:05 p.m.
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, January 10, 2014 2:36 AM
> To: David Laight
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; Williams, Mitch A;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; Brandeburg,
> Jesse
> Subject: Re: [net-next 05/16] i40e: fix long lines
> 
> On Fri, 2014-01-10 at 10:02 +0000, David Laight wrote:
> > > Another way this could be changed is to put the
> > > return value on a separate line like:
> > >
> > > i40e_status
> > > i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool
> set,
> > > 				    struct i40e_asq_cmd_details *cmd_details)
> >
> > Personally I prefer that so I can find the definition by grepping
> > for '^function_name' - but it doesn't seem to be done in any linux
> > source files.
> 
> That form is used in a lot of files.
> $ git grep -E "^[a-z_]+\("
> 

So, Joe, I just want to be clear. Is this a NAK, or just an FYI? If it's a serious issue for you (or Dave), we'll fix it right away. Otherwise, I'll just keep it in mind and come back with a janitorial patch once we've got all of the driver functionality sorted.

-Mitch
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 10, 2014, 6:14 p.m.
On Fri, 2014-01-10 at 18:05 +0000, Williams, Mitch A wrote:
> > -----Original Message-----
> > From: Joe Perches [mailto:joe@perches.com]
> > Sent: Friday, January 10, 2014 2:36 AM
[]
> > On Fri, 2014-01-10 at 10:02 +0000, David Laight wrote:
> > > > Another way this could be changed is to put the
> > > > return value on a separate line like:
> > > >
> > > > i40e_status
> > > > i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool
> > set,
> > > > 				    struct i40e_asq_cmd_details *cmd_details)
> > >
> > > Personally I prefer that so I can find the definition by grepping
> > > for '^function_name' - but it doesn't seem to be done in any linux
> > > source files.
> > 
> > That form is used in a lot of files.
> > $ git grep -E "^[a-z_]+\("
> > 
> 
> So, Joe, I just want to be clear. Is this a NAK, or just an FYI?

Hi Mitch.

This is your/intel's code.  I don't/shouldn't nak it.
I made a simple suggestion of an alternate style.

> If it's a serious issue for you (or Dave), we'll fix it right away.

It's up to you all to fix it (or not) as you chose.
It's trivial to me.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Williams, Mitch A - Jan. 10, 2014, 6:20 p.m.
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Friday, January 10, 2014 10:15 AM
> To: Williams, Mitch A
> Cc: David Laight; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; Brandeburg,
> Jesse
> Subject: Re: [net-next 05/16] i40e: fix long lines
> 
> On Fri, 2014-01-10 at 18:05 +0000, Williams, Mitch A wrote:
> > > -----Original Message-----
> > > From: Joe Perches [mailto:joe@perches.com]
> > > Sent: Friday, January 10, 2014 2:36 AM
> []
> > > On Fri, 2014-01-10 at 10:02 +0000, David Laight wrote:
> > > > > Another way this could be changed is to put the
> > > > > return value on a separate line like:
> > > > >
> > > > > i40e_status
> > > > > i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid,
> bool
> > > set,
> > > > > 				    struct i40e_asq_cmd_details *cmd_details)
> > > >
> > > > Personally I prefer that so I can find the definition by grepping
> > > > for '^function_name' - but it doesn't seem to be done in any linux
> > > > source files.
> > >
> > > That form is used in a lot of files.
> > > $ git grep -E "^[a-z_]+\("
> > >
> >
> > So, Joe, I just want to be clear. Is this a NAK, or just an FYI?
> 
> Hi Mitch.
> 
> This is your/intel's code.  I don't/shouldn't nak it.
> I made a simple suggestion of an alternate style.
> 
> > If it's a serious issue for you (or Dave), we'll fix it right away.
> 
> It's up to you all to fix it (or not) as you chose.
> It's trivial to me.
> 
> cheers, Joe

Thanks, Joe. As long as Dave is OK with it, I'll let it stand as-is and scrub the whole driver later for stuff like this.

BTW, I appreciate all the time you spend looking at our code. We've got a decent sized team here, but it's always good to have somebody outside the organization keeping us honest.

-Mitch
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 6bfbeec..1bf0ec1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -681,7 +681,8 @@  aq_add_vsi_exit:
  * @cmd_details: pointer to command details structure or NULL
  **/
 i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
-				u16 seid, bool set, struct i40e_asq_cmd_details *cmd_details)
+				u16 seid, bool set,
+				struct i40e_asq_cmd_details *cmd_details)
 {
 	struct i40e_aq_desc desc;
 	struct i40e_aqc_set_vsi_promiscuous_modes *cmd =
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index c7c3d82..f8c53a6 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -93,9 +93,9 @@  i40e_status i40e_aq_set_vsi_broadcast(struct i40e_hw *hw,
 				u16 vsi_id, bool set_filter,
 				struct i40e_asq_cmd_details *cmd_details);
 i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
-				u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
+		u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
 i40e_status i40e_aq_set_vsi_multicast_promiscuous(struct i40e_hw *hw,
-				u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
+		u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
 i40e_status i40e_aq_get_vsi_params(struct i40e_hw *hw,
 				struct i40e_vsi_context *vsi_ctx,
 				struct i40e_asq_cmd_details *cmd_details);