| Message ID | 1389344336-1558-6-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
> > 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
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
> -----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
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
> -----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
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);