* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
@ 2015-04-08 11:43 ` Neil Horman
2015-04-08 12:16 ` Butler, Siobhan A
2015-04-08 14:16 ` Wiles, Keith
` (6 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-08 11:43 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> Hi all,
> To add to the tools brainstorming - I propose we use the following Coding Standards as the basis of guidelines on coding style going forward.
> The style outlined below is in alignment with the current convention used for the majority of the project.
> Any thoughts/suggestions or feedback welcome.
> Thanks
> Siobhan :)
> <siobhan.a.butler@intel.com>
>
>
>
> Coding Style
> ~~~~~~~~~~
>
> Description
> -----------
>
> This document specifies the preferred style for source files in the DPDK source tree.
> It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2 Kernel Developer's Manual (see man style(9)),
> but was heavily modified for the needs of the DPDK. Many of the style rules are implicit in the examples.
> Be careful to check the examples before assuming that style is silent on an issue.
>
> General Guidelines
> ------------------
>
> The rules and guidelines given in this document cannot cover every situation, so the following general guidelines should be used as a fallback:
> The code style should be consistent within each individual file, and within each file in a given directory or module - in the case of creating new files
> The primary reason for coding standards is to increase code readability and comprehensibility, therefore always use whatever option will make the code easiest to read.
>
> The following more specific recommendations apply to all sections, both for C and assembly code:
> Line length is recommended to be not more than 80 characters, including comments. [Tab stop size should be assumed to be at least 4-characters wide]
> Indentation should be to no more than 3 levels deep.
> NOTE The above are recommendations, and not hard limits. However, it is expected that the recommendations should be followed in all but the rarest situations.
> C Comment Style
>
> Usual Comments
> --------------
>
> These comments should be used in normal cases. To document a public API, a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
>
> License Header
> --------------
>
> Each file should begin with a special comment tag which will contain the appropriate copyright and license for the file (Generally BSD License).
> After any copyright header, a blank line should be left before any other contents, e.g. include statements in a C file.
>
This can become very confusing. There already is a LICENSE.GPL and LICENSE.LGPL
file at the top of the project, allowing others to insert their own licenses
within their files can make it difficult for end user to determine if it is
legally safe to use a given project.
I'd suggest instead that contributions be disallowed from including license
files in their code, relying instead on only a single license at the top of the
project (which should likely be BSD or LGPL, since we're shipping a library).
IANAL, but it seems to me to be dangerous to do anything else. For example, all
the code that is dual licensed in the library (like rte_pci_dev_ids.h). It
allows for a BSD or GPL license. If someone builds dpdk as a DSO and
distributes it under the former, every application that links against that
re-distribution may arguably itself become GPL licensed. While I'm personally
fine with that, I can see it as being a big deal to some end users. Unifying
the license makes the re-distribution license issue more clear for everyone.
Neil
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 11:43 ` Neil Horman
@ 2015-04-08 12:16 ` Butler, Siobhan A
2015-04-08 12:20 ` Butler, Siobhan A
2015-04-08 13:11 ` Neil Horman
0 siblings, 2 replies; 62+ messages in thread
From: Butler, Siobhan A @ 2015-04-08 12:16 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, April 8, 2015 12:44 PM
> To: Butler, Siobhan A
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
>
> On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> > Hi all,
> > To add to the tools brainstorming - I propose we use the following Coding
> Standards as the basis of guidelines on coding style going forward.
> > The style outlined below is in alignment with the current convention used
> for the majority of the project.
> > Any thoughts/suggestions or feedback welcome.
> > Thanks
> > Siobhan :)
> > <siobhan.a.butler@intel.com>
> >
> >
> >
> > Coding Style
> > ~~~~~~~~~~
> >
> > Description
> > -----------
> >
> > This document specifies the preferred style for source files in the DPDK
> source tree.
> > It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
> > Kernel Developer's Manual (see man style(9)), but was heavily modified for
> the needs of the DPDK. Many of the style rules are implicit in the examples.
> > Be careful to check the examples before assuming that style is silent on an
> issue.
> >
> > General Guidelines
> > ------------------
> >
> > The rules and guidelines given in this document cannot cover every
> situation, so the following general guidelines should be used as a fallback:
> > The code style should be consistent within each individual file, and
> > within each file in a given directory or module - in the case of creating new
> files The primary reason for coding standards is to increase code readability
> and comprehensibility, therefore always use whatever option will make the
> code easiest to read.
> >
> > The following more specific recommendations apply to all sections, both for
> C and assembly code:
> > Line length is recommended to be not more than 80 characters,
> > including comments. [Tab stop size should be assumed to be at least 4-
> characters wide] Indentation should be to no more than 3 levels deep.
> > NOTE The above are recommendations, and not hard limits. However, it is
> expected that the recommendations should be followed in all but the rarest
> situations.
> > C Comment Style
> >
> > Usual Comments
> > --------------
> >
> > These comments should be used in normal cases. To document a public
> API, a doxygen-like format must be used: refer to Doxygen Documentation.
> > /*
> > * VERY important single-line comments look like this.
> > */
> >
> > /* Most single-line comments look like this. */
> >
> > /*
> > * Multi-line comments look like this. Make them real sentences. Fill
> > * them so they look like real paragraphs.
> > */
> >
> > License Header
> > --------------
> >
> > Each file should begin with a special comment tag which will contain the
> appropriate copyright and license for the file (Generally BSD License).
> > After any copyright header, a blank line should be left before any other
> contents, e.g. include statements in a C file.
> >
>
> This can become very confusing. There already is a LICENSE.GPL and
> LICENSE.LGPL file at the top of the project, allowing others to insert their
> own licenses within their files can make it difficult for end user to determine
> if it is legally safe to use a given project.
>
> I'd suggest instead that contributions be disallowed from including license
> files in their code, relying instead on only a single license at the top of the
> project (which should likely be BSD or LGPL, since we're shipping a library).
>
> IANAL, but it seems to me to be dangerous to do anything else. For
> example, all the code that is dual licensed in the library (like
> rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone builds
> dpdk as a DSO and distributes it under the former, every application that links
> against that re-distribution may arguably itself become GPL licensed. While
> I'm personally fine with that, I can see it as being a big deal to some end
> users. Unifying the license makes the re-distribution license issue more clear
> for everyone.
>
> Neil
Input appreciated Neil thank you, would it be best to include this in one of the community conference calls?
IANAL either ( yet at least :) ) - we can certainly consult with someone who has the expertise.
If others are interested in discussing this we can get added to the agenda for an upcoming call.
Is more detailed explanation/notice on the licensing structure required?
Thanks,
Siobhan
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 12:16 ` Butler, Siobhan A
@ 2015-04-08 12:20 ` Butler, Siobhan A
2015-04-08 13:11 ` Neil Horman
1 sibling, 0 replies; 62+ messages in thread
From: Butler, Siobhan A @ 2015-04-08 12:20 UTC (permalink / raw)
To: Butler, Siobhan A, Neil Horman; +Cc: dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Butler, Siobhan A
> Sent: Wednesday, April 8, 2015 1:16 PM
> To: Neil Horman
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
>
>
>
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Wednesday, April 8, 2015 12:44 PM
> > To: Butler, Siobhan A
> > Cc: Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] tools brainstorming
> >
> > On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> > > Hi all,
> > > To add to the tools brainstorming - I propose we use the following
> > > Coding
> > Standards as the basis of guidelines on coding style going forward.
> > > The style outlined below is in alignment with the current convention
> > > used
> > for the majority of the project.
> > > Any thoughts/suggestions or feedback welcome.
> > > Thanks
> > > Siobhan :)
> > > <siobhan.a.butler@intel.com>
> > >
> > >
> > >
> > > Coding Style
> > > ~~~~~~~~~~
> > >
> > > Description
> > > -----------
> > >
> > > This document specifies the preferred style for source files in the
> > > DPDK
> > source tree.
> > > It is based on the Linux Kernel coding guidelines and the FreeBSD
> > > 7.2 Kernel Developer's Manual (see man style(9)), but was heavily
> > > modified for
> > the needs of the DPDK. Many of the style rules are implicit in the examples.
> > > Be careful to check the examples before assuming that style is
> > > silent on an
> > issue.
> > >
> > > General Guidelines
> > > ------------------
> > >
> > > The rules and guidelines given in this document cannot cover every
> > situation, so the following general guidelines should be used as a fallback:
> > > The code style should be consistent within each individual file, and
> > > within each file in a given directory or module - in the case of
> > > creating new
> > files The primary reason for coding standards is to increase code
> > readability and comprehensibility, therefore always use whatever
> > option will make the code easiest to read.
> > >
> > > The following more specific recommendations apply to all sections,
> > > both for
> > C and assembly code:
> > > Line length is recommended to be not more than 80 characters,
> > > including comments. [Tab stop size should be assumed to be at least
> > > 4-
> > characters wide] Indentation should be to no more than 3 levels deep.
> > > NOTE The above are recommendations, and not hard limits. However, it
> > > is
> > expected that the recommendations should be followed in all but the
> > rarest situations.
> > > C Comment Style
> > >
> > > Usual Comments
> > > --------------
> > >
> > > These comments should be used in normal cases. To document a public
> > API, a doxygen-like format must be used: refer to Doxygen
> Documentation.
> > > /*
> > > * VERY important single-line comments look like this.
> > > */
> > >
> > > /* Most single-line comments look like this. */
> > >
> > > /*
> > > * Multi-line comments look like this. Make them real sentences. Fill
> > > * them so they look like real paragraphs.
> > > */
> > >
> > > License Header
> > > --------------
> > >
> > > Each file should begin with a special comment tag which will contain
> > > the
> > appropriate copyright and license for the file (Generally BSD License).
> > > After any copyright header, a blank line should be left before any
> > > other
> > contents, e.g. include statements in a C file.
> > >
Apologies, my mistake the tag format is now out of date - and should be removed from the style guide.
Siobhan
> >
> > This can become very confusing. There already is a LICENSE.GPL and
> > LICENSE.LGPL file at the top of the project, allowing others to insert
> > their own licenses within their files can make it difficult for end
> > user to determine if it is legally safe to use a given project.
> >
> > I'd suggest instead that contributions be disallowed from including
> > license files in their code, relying instead on only a single license
> > at the top of the project (which should likely be BSD or LGPL, since we're
> shipping a library).
> >
> > IANAL, but it seems to me to be dangerous to do anything else. For
> > example, all the code that is dual licensed in the library (like
> > rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone
> > builds dpdk as a DSO and distributes it under the former, every
> > application that links against that re-distribution may arguably
> > itself become GPL licensed. While I'm personally fine with that, I
> > can see it as being a big deal to some end users. Unifying the
> > license makes the re-distribution license issue more clear for everyone.
> >
> > Neil
>
>
> Input appreciated Neil thank you, would it be best to include this in one of
> the community conference calls?
> IANAL either ( yet at least :) ) - we can certainly consult with someone who
> has the expertise.
> If others are interested in discussing this we can get added to the agenda for
> an upcoming call.
>
> Is more detailed explanation/notice on the licensing structure required?
> Thanks,
> Siobhan
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 12:16 ` Butler, Siobhan A
2015-04-08 12:20 ` Butler, Siobhan A
@ 2015-04-08 13:11 ` Neil Horman
2015-04-08 14:40 ` Butler, Siobhan A
1 sibling, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-08 13:11 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
On Wed, Apr 08, 2015 at 12:16:10PM +0000, Butler, Siobhan A wrote:
>
>
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Wednesday, April 8, 2015 12:44 PM
> > To: Butler, Siobhan A
> > Cc: Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] tools brainstorming
> >
> > On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> > > Hi all,
> > > To add to the tools brainstorming - I propose we use the following Coding
> > Standards as the basis of guidelines on coding style going forward.
> > > The style outlined below is in alignment with the current convention used
> > for the majority of the project.
> > > Any thoughts/suggestions or feedback welcome.
> > > Thanks
> > > Siobhan :)
> > > <siobhan.a.butler@intel.com>
> > >
> > >
> > >
> > > Coding Style
> > > ~~~~~~~~~~
> > >
> > > Description
> > > -----------
> > >
> > > This document specifies the preferred style for source files in the DPDK
> > source tree.
> > > It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
> > > Kernel Developer's Manual (see man style(9)), but was heavily modified for
> > the needs of the DPDK. Many of the style rules are implicit in the examples.
> > > Be careful to check the examples before assuming that style is silent on an
> > issue.
> > >
> > > General Guidelines
> > > ------------------
> > >
> > > The rules and guidelines given in this document cannot cover every
> > situation, so the following general guidelines should be used as a fallback:
> > > The code style should be consistent within each individual file, and
> > > within each file in a given directory or module - in the case of creating new
> > files The primary reason for coding standards is to increase code readability
> > and comprehensibility, therefore always use whatever option will make the
> > code easiest to read.
> > >
> > > The following more specific recommendations apply to all sections, both for
> > C and assembly code:
> > > Line length is recommended to be not more than 80 characters,
> > > including comments. [Tab stop size should be assumed to be at least 4-
> > characters wide] Indentation should be to no more than 3 levels deep.
> > > NOTE The above are recommendations, and not hard limits. However, it is
> > expected that the recommendations should be followed in all but the rarest
> > situations.
> > > C Comment Style
> > >
> > > Usual Comments
> > > --------------
> > >
> > > These comments should be used in normal cases. To document a public
> > API, a doxygen-like format must be used: refer to Doxygen Documentation.
> > > /*
> > > * VERY important single-line comments look like this.
> > > */
> > >
> > > /* Most single-line comments look like this. */
> > >
> > > /*
> > > * Multi-line comments look like this. Make them real sentences. Fill
> > > * them so they look like real paragraphs.
> > > */
> > >
> > > License Header
> > > --------------
> > >
> > > Each file should begin with a special comment tag which will contain the
> > appropriate copyright and license for the file (Generally BSD License).
> > > After any copyright header, a blank line should be left before any other
> > contents, e.g. include statements in a C file.
> > >
> >
> > This can become very confusing. There already is a LICENSE.GPL and
> > LICENSE.LGPL file at the top of the project, allowing others to insert their
> > own licenses within their files can make it difficult for end user to determine
> > if it is legally safe to use a given project.
> >
> > I'd suggest instead that contributions be disallowed from including license
> > files in their code, relying instead on only a single license at the top of the
> > project (which should likely be BSD or LGPL, since we're shipping a library).
> >
> > IANAL, but it seems to me to be dangerous to do anything else. For
> > example, all the code that is dual licensed in the library (like
> > rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone builds
> > dpdk as a DSO and distributes it under the former, every application that links
> > against that re-distribution may arguably itself become GPL licensed. While
> > I'm personally fine with that, I can see it as being a big deal to some end
> > users. Unifying the license makes the re-distribution license issue more clear
> > for everyone.
> >
> > Neil
>
>
> Input appreciated Neil thank you, would it be best to include this in one of the community conference calls?
> IANAL either ( yet at least :) ) - we can certainly consult with someone who has the expertise.
> If others are interested in discussing this we can get added to the agenda for an upcoming call.
>
> Is more detailed explanation/notice on the licensing structure required?
> Thanks,
> Siobhan
>
If you want to discuss it on the community call I think that would be fine,
certainly, but it seems that on this forum is the real place to encourage
conversation. Its recorded for posterity, and is open to the entire community,
all we need are people to speak up.
Neil
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 13:11 ` Neil Horman
@ 2015-04-08 14:40 ` Butler, Siobhan A
2015-04-08 15:39 ` Neil Horman
2015-04-08 22:29 ` Jay Rolette
0 siblings, 2 replies; 62+ messages in thread
From: Butler, Siobhan A @ 2015-04-08 14:40 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, April 8, 2015 2:11 PM
> To: Butler, Siobhan A
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
>
> On Wed, Apr 08, 2015 at 12:16:10PM +0000, Butler, Siobhan A wrote:
> >
> >
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Wednesday, April 8, 2015 12:44 PM
> > > To: Butler, Siobhan A
> > > Cc: Thomas Monjalon; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] tools brainstorming
> > >
> > > On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> > > > Hi all,
> > > > To add to the tools brainstorming - I propose we use the following
> > > > Coding
> > > Standards as the basis of guidelines on coding style going forward.
> > > > The style outlined below is in alignment with the current
> > > > convention used
> > > for the majority of the project.
> > > > Any thoughts/suggestions or feedback welcome.
> > > > Thanks
> > > > Siobhan :)
> > > > <siobhan.a.butler@intel.com>
> > > >
> > > >
> > > >
> > > > Coding Style
> > > > ~~~~~~~~~~
> > > >
> > > > Description
> > > > -----------
> > > >
> > > > This document specifies the preferred style for source files in
> > > > the DPDK
> > > source tree.
> > > > It is based on the Linux Kernel coding guidelines and the FreeBSD
> > > > 7.2 Kernel Developer's Manual (see man style(9)), but was heavily
> > > > modified for
> > > the needs of the DPDK. Many of the style rules are implicit in the
> examples.
> > > > Be careful to check the examples before assuming that style is
> > > > silent on an
> > > issue.
> > > >
> > > > General Guidelines
> > > > ------------------
> > > >
> > > > The rules and guidelines given in this document cannot cover every
> > > situation, so the following general guidelines should be used as a fallback:
> > > > The code style should be consistent within each individual file,
> > > > and within each file in a given directory or module - in the case
> > > > of creating new
> > > files The primary reason for coding standards is to increase code
> > > readability and comprehensibility, therefore always use whatever
> > > option will make the code easiest to read.
> > > >
> > > > The following more specific recommendations apply to all sections,
> > > > both for
> > > C and assembly code:
> > > > Line length is recommended to be not more than 80 characters,
> > > > including comments. [Tab stop size should be assumed to be at
> > > > least 4-
> > > characters wide] Indentation should be to no more than 3 levels deep.
> > > > NOTE The above are recommendations, and not hard limits. However,
> > > > it is
> > > expected that the recommendations should be followed in all but the
> > > rarest situations.
> > > > C Comment Style
> > > >
> > > > Usual Comments
> > > > --------------
> > > >
> > > > These comments should be used in normal cases. To document a
> > > > public
> > > API, a doxygen-like format must be used: refer to Doxygen
> Documentation.
> > > > /*
> > > > * VERY important single-line comments look like this.
> > > > */
> > > >
> > > > /* Most single-line comments look like this. */
> > > >
> > > > /*
> > > > * Multi-line comments look like this. Make them real sentences. Fill
> > > > * them so they look like real paragraphs.
> > > > */
> > > >
> > > > License Header
> > > > --------------
> > > >
> > > > Each file should begin with a special comment tag which will
> > > > contain the
> > > appropriate copyright and license for the file (Generally BSD License).
> > > > After any copyright header, a blank line should be left before any
> > > > other
> > > contents, e.g. include statements in a C file.
> > > >
> > >
> > > This can become very confusing. There already is a LICENSE.GPL and
> > > LICENSE.LGPL file at the top of the project, allowing others to
> > > insert their own licenses within their files can make it difficult
> > > for end user to determine if it is legally safe to use a given project.
> > >
> > > I'd suggest instead that contributions be disallowed from including
> > > license files in their code, relying instead on only a single
> > > license at the top of the project (which should likely be BSD or LGPL, since
> we're shipping a library).
> > >
> > > IANAL, but it seems to me to be dangerous to do anything else. For
> > > example, all the code that is dual licensed in the library (like
> > > rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone
> > > builds dpdk as a DSO and distributes it under the former, every
> > > application that links against that re-distribution may arguably
> > > itself become GPL licensed. While I'm personally fine with that, I
> > > can see it as being a big deal to some end users. Unifying the
> > > license makes the re-distribution license issue more clear for everyone.
> > >
> > > Neil
> >
> >
> > Input appreciated Neil thank you, would it be best to include this in one of
> the community conference calls?
> > IANAL either ( yet at least :) ) - we can certainly consult with someone who
> has the expertise.
> > If others are interested in discussing this we can get added to the agenda
> for an upcoming call.
> >
> > Is more detailed explanation/notice on the licensing structure required?
> > Thanks,
> > Siobhan
> >
> If you want to discuss it on the community call I think that would be fine,
> certainly, but it seems that on this forum is the real place to encourage
> conversation. Its recorded for posterity, and is open to the entire
> community, all we need are people to speak up.
>
> Neil
Fair enough - no issue with that either.
The license section aside, do you think the coding style is ok?
S
>
> >
> >
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 14:40 ` Butler, Siobhan A
@ 2015-04-08 15:39 ` Neil Horman
2015-04-08 22:29 ` Jay Rolette
1 sibling, 0 replies; 62+ messages in thread
From: Neil Horman @ 2015-04-08 15:39 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
On Wed, Apr 08, 2015 at 02:40:59PM +0000, Butler, Siobhan A wrote:
>
>
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Wednesday, April 8, 2015 2:11 PM
> > To: Butler, Siobhan A
> > Cc: Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] tools brainstorming
> >
> > On Wed, Apr 08, 2015 at 12:16:10PM +0000, Butler, Siobhan A wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > Sent: Wednesday, April 8, 2015 12:44 PM
> > > > To: Butler, Siobhan A
> > > > Cc: Thomas Monjalon; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] tools brainstorming
> > > >
> > > > On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> > > > > Hi all,
> > > > > To add to the tools brainstorming - I propose we use the following
> > > > > Coding
> > > > Standards as the basis of guidelines on coding style going forward.
> > > > > The style outlined below is in alignment with the current
> > > > > convention used
> > > > for the majority of the project.
> > > > > Any thoughts/suggestions or feedback welcome.
> > > > > Thanks
> > > > > Siobhan :)
> > > > > <siobhan.a.butler@intel.com>
> > > > >
> > > > >
> > > > >
> > > > > Coding Style
> > > > > ~~~~~~~~~~
> > > > >
> > > > > Description
> > > > > -----------
> > > > >
> > > > > This document specifies the preferred style for source files in
> > > > > the DPDK
> > > > source tree.
> > > > > It is based on the Linux Kernel coding guidelines and the FreeBSD
> > > > > 7.2 Kernel Developer's Manual (see man style(9)), but was heavily
> > > > > modified for
> > > > the needs of the DPDK. Many of the style rules are implicit in the
> > examples.
> > > > > Be careful to check the examples before assuming that style is
> > > > > silent on an
> > > > issue.
> > > > >
> > > > > General Guidelines
> > > > > ------------------
> > > > >
> > > > > The rules and guidelines given in this document cannot cover every
> > > > situation, so the following general guidelines should be used as a fallback:
> > > > > The code style should be consistent within each individual file,
> > > > > and within each file in a given directory or module - in the case
> > > > > of creating new
> > > > files The primary reason for coding standards is to increase code
> > > > readability and comprehensibility, therefore always use whatever
> > > > option will make the code easiest to read.
> > > > >
> > > > > The following more specific recommendations apply to all sections,
> > > > > both for
> > > > C and assembly code:
> > > > > Line length is recommended to be not more than 80 characters,
> > > > > including comments. [Tab stop size should be assumed to be at
> > > > > least 4-
> > > > characters wide] Indentation should be to no more than 3 levels deep.
> > > > > NOTE The above are recommendations, and not hard limits. However,
> > > > > it is
> > > > expected that the recommendations should be followed in all but the
> > > > rarest situations.
> > > > > C Comment Style
> > > > >
> > > > > Usual Comments
> > > > > --------------
> > > > >
> > > > > These comments should be used in normal cases. To document a
> > > > > public
> > > > API, a doxygen-like format must be used: refer to Doxygen
> > Documentation.
> > > > > /*
> > > > > * VERY important single-line comments look like this.
> > > > > */
> > > > >
> > > > > /* Most single-line comments look like this. */
> > > > >
> > > > > /*
> > > > > * Multi-line comments look like this. Make them real sentences. Fill
> > > > > * them so they look like real paragraphs.
> > > > > */
> > > > >
> > > > > License Header
> > > > > --------------
> > > > >
> > > > > Each file should begin with a special comment tag which will
> > > > > contain the
> > > > appropriate copyright and license for the file (Generally BSD License).
> > > > > After any copyright header, a blank line should be left before any
> > > > > other
> > > > contents, e.g. include statements in a C file.
> > > > >
> > > >
> > > > This can become very confusing. There already is a LICENSE.GPL and
> > > > LICENSE.LGPL file at the top of the project, allowing others to
> > > > insert their own licenses within their files can make it difficult
> > > > for end user to determine if it is legally safe to use a given project.
> > > >
> > > > I'd suggest instead that contributions be disallowed from including
> > > > license files in their code, relying instead on only a single
> > > > license at the top of the project (which should likely be BSD or LGPL, since
> > we're shipping a library).
> > > >
> > > > IANAL, but it seems to me to be dangerous to do anything else. For
> > > > example, all the code that is dual licensed in the library (like
> > > > rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone
> > > > builds dpdk as a DSO and distributes it under the former, every
> > > > application that links against that re-distribution may arguably
> > > > itself become GPL licensed. While I'm personally fine with that, I
> > > > can see it as being a big deal to some end users. Unifying the
> > > > license makes the re-distribution license issue more clear for everyone.
> > > >
> > > > Neil
> > >
> > >
> > > Input appreciated Neil thank you, would it be best to include this in one of
> > the community conference calls?
> > > IANAL either ( yet at least :) ) - we can certainly consult with someone who
> > has the expertise.
> > > If others are interested in discussing this we can get added to the agenda
> > for an upcoming call.
> > >
> > > Is more detailed explanation/notice on the licensing structure required?
> > > Thanks,
> > > Siobhan
> > >
> > If you want to discuss it on the community call I think that would be fine,
> > certainly, but it seems that on this forum is the real place to encourage
> > conversation. Its recorded for posterity, and is open to the entire
> > community, all we need are people to speak up.
> >
> > Neil
> Fair enough - no issue with that either.
> The license section aside, do you think the coding style is ok?
I've not looked at it yet, this just jumped out at me. i'll try get to it this
afternoon.
Neil
> S
> >
> > >
> > >
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 14:40 ` Butler, Siobhan A
2015-04-08 15:39 ` Neil Horman
@ 2015-04-08 22:29 ` Jay Rolette
2015-04-08 22:38 ` Stephen Hemminger
1 sibling, 1 reply; 62+ messages in thread
From: Jay Rolette @ 2015-04-08 22:29 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
"C comments" includes //, right? It's been part of the C standard for a long time now...
Sent from my iPhone
> On Apr 8, 2015, at 8:40 AM, Butler, Siobhan A <siobhan.a.butler@intel.com> wrote:
>
>
>
>> -----Original Message-----
>> From: Neil Horman [mailto:nhorman@tuxdriver.com]
>> Sent: Wednesday, April 8, 2015 2:11 PM
>> To: Butler, Siobhan A
>> Cc: Thomas Monjalon; dev@dpdk.org
>> Subject: Re: [dpdk-dev] tools brainstorming
>>
>>> On Wed, Apr 08, 2015 at 12:16:10PM +0000, Butler, Siobhan A wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Neil Horman [mailto:nhorman@tuxdriver.com]
>>>> Sent: Wednesday, April 8, 2015 12:44 PM
>>>> To: Butler, Siobhan A
>>>> Cc: Thomas Monjalon; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] tools brainstorming
>>>>
>>>>> On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
>>>>> Hi all,
>>>>> To add to the tools brainstorming - I propose we use the following
>>>>> Coding
>>>> Standards as the basis of guidelines on coding style going forward.
>>>>> The style outlined below is in alignment with the current
>>>>> convention used
>>>> for the majority of the project.
>>>>> Any thoughts/suggestions or feedback welcome.
>>>>> Thanks
>>>>> Siobhan :)
>>>>> <siobhan.a.butler@intel.com>
>>>>>
>>>>>
>>>>>
>>>>> Coding Style
>>>>> ~~~~~~~~~~
>>>>>
>>>>> Description
>>>>> -----------
>>>>>
>>>>> This document specifies the preferred style for source files in
>>>>> the DPDK
>>>> source tree.
>>>>> It is based on the Linux Kernel coding guidelines and the FreeBSD
>>>>> 7.2 Kernel Developer's Manual (see man style(9)), but was heavily
>>>>> modified for
>>>> the needs of the DPDK. Many of the style rules are implicit in the
>> examples.
>>>>> Be careful to check the examples before assuming that style is
>>>>> silent on an
>>>> issue.
>>>>>
>>>>> General Guidelines
>>>>> ------------------
>>>>>
>>>>> The rules and guidelines given in this document cannot cover every
>>>> situation, so the following general guidelines should be used as a fallback:
>>>>> The code style should be consistent within each individual file,
>>>>> and within each file in a given directory or module - in the case
>>>>> of creating new
>>>> files The primary reason for coding standards is to increase code
>>>> readability and comprehensibility, therefore always use whatever
>>>> option will make the code easiest to read.
>>>>>
>>>>> The following more specific recommendations apply to all sections,
>>>>> both for
>>>> C and assembly code:
>>>>> Line length is recommended to be not more than 80 characters,
>>>>> including comments. [Tab stop size should be assumed to be at
>>>>> least 4-
>>>> characters wide] Indentation should be to no more than 3 levels deep.
>>>>> NOTE The above are recommendations, and not hard limits. However,
>>>>> it is
>>>> expected that the recommendations should be followed in all but the
>>>> rarest situations.
>>>>> C Comment Style
>>>>>
>>>>> Usual Comments
>>>>> --------------
>>>>>
>>>>> These comments should be used in normal cases. To document a
>>>>> public
>>>> API, a doxygen-like format must be used: refer to Doxygen
>> Documentation.
>>>>> /*
>>>>> * VERY important single-line comments look like this.
>>>>> */
>>>>>
>>>>> /* Most single-line comments look like this. */
>>>>>
>>>>> /*
>>>>> * Multi-line comments look like this. Make them real sentences. Fill
>>>>> * them so they look like real paragraphs.
>>>>> */
>>>>>
>>>>> License Header
>>>>> --------------
>>>>>
>>>>> Each file should begin with a special comment tag which will
>>>>> contain the
>>>> appropriate copyright and license for the file (Generally BSD License).
>>>>> After any copyright header, a blank line should be left before any
>>>>> other
>>>> contents, e.g. include statements in a C file.
>>>>
>>>> This can become very confusing. There already is a LICENSE.GPL and
>>>> LICENSE.LGPL file at the top of the project, allowing others to
>>>> insert their own licenses within their files can make it difficult
>>>> for end user to determine if it is legally safe to use a given project.
>>>>
>>>> I'd suggest instead that contributions be disallowed from including
>>>> license files in their code, relying instead on only a single
>>>> license at the top of the project (which should likely be BSD or LGPL, since
>> we're shipping a library).
>>>>
>>>> IANAL, but it seems to me to be dangerous to do anything else. For
>>>> example, all the code that is dual licensed in the library (like
>>>> rte_pci_dev_ids.h). It allows for a BSD or GPL license. If someone
>>>> builds dpdk as a DSO and distributes it under the former, every
>>>> application that links against that re-distribution may arguably
>>>> itself become GPL licensed. While I'm personally fine with that, I
>>>> can see it as being a big deal to some end users. Unifying the
>>>> license makes the re-distribution license issue more clear for everyone.
>>>>
>>>> Neil
>>>
>>>
>>> Input appreciated Neil thank you, would it be best to include this in one of
>> the community conference calls?
>>> IANAL either ( yet at least :) ) - we can certainly consult with someone who
>> has the expertise.
>>> If others are interested in discussing this we can get added to the agenda
>> for an upcoming call.
>>>
>>> Is more detailed explanation/notice on the licensing structure required?
>>> Thanks,
>>> Siobhan
>> If you want to discuss it on the community call I think that would be fine,
>> certainly, but it seems that on this forum is the real place to encourage
>> conversation. Its recorded for posterity, and is open to the entire
>> community, all we need are people to speak up.
>>
>> Neil
> Fair enough - no issue with that either.
> The license section aside, do you think the coding style is ok?
> S
>>
>>>
>>>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 22:29 ` Jay Rolette
@ 2015-04-08 22:38 ` Stephen Hemminger
2015-04-09 16:31 ` Jay Rolette
0 siblings, 1 reply; 62+ messages in thread
From: Stephen Hemminger @ 2015-04-08 22:38 UTC (permalink / raw)
To: Jay Rolette; +Cc: dev
On Wed, 8 Apr 2015 16:29:54 -0600
Jay Rolette <rolette@infiniteio.com> wrote:
> "C comments" includes //, right? It's been part of the C standard for a long time now...
Yes but.
I like to use checkpatch and checkpatch enforces kernel style which does not allow // for
comments.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 22:38 ` Stephen Hemminger
@ 2015-04-09 16:31 ` Jay Rolette
2015-04-09 19:16 ` Neil Horman
0 siblings, 1 reply; 62+ messages in thread
From: Jay Rolette @ 2015-04-09 16:31 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Wed, 8 Apr 2015 16:29:54 -0600
> Jay Rolette <rolette@infiniteio.com> wrote:
>
> > "C comments" includes //, right? It's been part of the C standard for a
> long time now...
>
> Yes but.
> I like to use checkpatch and checkpatch enforces kernel style which does
> not allow // for
> comments.
>
Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
requirement to follow all of its rules
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 16:31 ` Jay Rolette
@ 2015-04-09 19:16 ` Neil Horman
2015-04-09 19:38 ` Jay Rolette
0 siblings, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-09 19:16 UTC (permalink / raw)
To: Jay Rolette; +Cc: dev
On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> stephen@networkplumber.org> wrote:
>
> > On Wed, 8 Apr 2015 16:29:54 -0600
> > Jay Rolette <rolette@infiniteio.com> wrote:
> >
> > > "C comments" includes //, right? It's been part of the C standard for a
> > long time now...
> >
> > Yes but.
> > I like to use checkpatch and checkpatch enforces kernel style which does
> > not allow // for
> > comments.
> >
>
> Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> requirement to follow all of its rules
>
Doesn't that beg the question, why? I understand the DPDK isn't the kernel, but
we're not talking about clarity of code, not anything functional to that code.
It seems we would be better served by just taking something that works here
rather than re-inventing the wheel and digging into the minuate of what type of
comments should be allowed (unless there is a compelling reason to change it
that supercedes the avilable tools). If not checkpath, then some other tool,
but It seems to me that coding style is one of those things where we can bend to
the tool rather than taking the time to make the tool do exactly whats desired,
at least until someone gets the time to modify it.
Neil
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 19:16 ` Neil Horman
@ 2015-04-09 19:38 ` Jay Rolette
2015-04-09 20:14 ` Neil Horman
2015-04-09 21:10 ` Wiles, Keith
0 siblings, 2 replies; 62+ messages in thread
From: Jay Rolette @ 2015-04-09 19:38 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> > stephen@networkplumber.org> wrote:
> >
> > > On Wed, 8 Apr 2015 16:29:54 -0600
> > > Jay Rolette <rolette@infiniteio.com> wrote:
> > >
> > > > "C comments" includes //, right? It's been part of the C standard
> for a
> > > long time now...
> > >
> > > Yes but.
> > > I like to use checkpatch and checkpatch enforces kernel style which
> does
> > > not allow // for
> > > comments.
> > >
> >
> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> > requirement to follow all of its rules
> >
>
> Doesn't that beg the question, why? I understand the DPDK isn't the
> kernel, but
> we're not talking about clarity of code, not anything functional to that
> code.
> It seems we would be better served by just taking something that works here
> rather than re-inventing the wheel and digging into the minuate of what
> type of
> comments should be allowed (unless there is a compelling reason to change
> it
> that supercedes the avilable tools). If not checkpath, then some other
> tool,
> but It seems to me that coding style is one of those things where we can
> bend to
> the tool rather than taking the time to make the tool do exactly whats
> desired,
> at least until someone gets the time to modify it.
>
Fair question.
It depends a bit on how much you want to encourage patch contributions. Is
it worth adding more pain for folks trying to contribute patches for things
like this?
Should we force someone to spend time redoing a patch because of which way
they do their parenthesis? What about number of spaces to indent code? //
vs /* */ comments? None of these matter functionally and they don't affect
maintenance generally.
If someone is modifying existing code, then yeah, they should follow the
prevailing style (indention level, brace alignment, etc.) of the file they
are in. It helps readability, which makes maintenance easier. However, IMO,
mixing // and /* */ for comments doesn't affect the readability of the
source.
I know if I submit a patch and the only feedback is that I should have used
/* */ for comments, I'm extremely unlikely spend extra time to resubmit the
patch for pedantry.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 19:38 ` Jay Rolette
@ 2015-04-09 20:14 ` Neil Horman
2015-04-09 21:10 ` Wiles, Keith
1 sibling, 0 replies; 62+ messages in thread
From: Neil Horman @ 2015-04-09 20:14 UTC (permalink / raw)
To: Jay Rolette; +Cc: dev
On Thu, Apr 09, 2015 at 02:38:32PM -0500, Jay Rolette wrote:
> On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>
> > On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> > > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> > > stephen@networkplumber.org> wrote:
> > >
> > > > On Wed, 8 Apr 2015 16:29:54 -0600
> > > > Jay Rolette <rolette@infiniteio.com> wrote:
> > > >
> > > > > "C comments" includes //, right? It's been part of the C standard
> > for a
> > > > long time now...
> > > >
> > > > Yes but.
> > > > I like to use checkpatch and checkpatch enforces kernel style which
> > does
> > > > not allow // for
> > > > comments.
> > > >
> > >
> > > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> > > requirement to follow all of its rules
> > >
> >
> > Doesn't that beg the question, why? I understand the DPDK isn't the
> > kernel, but
> > we're not talking about clarity of code, not anything functional to that
> > code.
> > It seems we would be better served by just taking something that works here
> > rather than re-inventing the wheel and digging into the minuate of what
> > type of
> > comments should be allowed (unless there is a compelling reason to change
> > it
> > that supercedes the avilable tools). If not checkpath, then some other
> > tool,
> > but It seems to me that coding style is one of those things where we can
> > bend to
> > the tool rather than taking the time to make the tool do exactly whats
> > desired,
> > at least until someone gets the time to modify it.
> >
>
> Fair question.
>
> It depends a bit on how much you want to encourage patch contributions. Is
> it worth adding more pain for folks trying to contribute patches for things
> like this?
>
> Should we force someone to spend time redoing a patch because of which way
> they do their parenthesis? What about number of spaces to indent code? //
> vs /* */ comments? None of these matter functionally and they don't affect
> maintenance generally.
>
> If someone is modifying existing code, then yeah, they should follow the
> prevailing style (indention level, brace alignment, etc.) of the file they
> are in. It helps readability, which makes maintenance easier. However, IMO,
> mixing // and /* */ for comments doesn't affect the readability of the
> source.
>
I take your meaning (that we shouldn't be overly restrictive on aspects of the
code that don't affect functionality, but I think this line of thinking quickly
spirals out into the question of weather to have coding styles at all. For any
aspect of code that you codify in a style guide, you are almost by definition
being restrictive:
// comments
vs.
/*
* comments
*/
or
void func(int args) {
}
vs.
void
func (int args)
{
}
Insert your own pet coding style variants as you see fit. If we want to enforce
coding styles, we have to pick something and enforce it. To suggest that we
allow both (or some subset of the entire set of some coding style aspect, as I
think you are trying to propose), while fine to do, somewhat calls into
question the need/desire for style guidlines at all. As you note below, you're
unlikely to revise a patch if the only comment is "use different commenting
style". The exact same might be a response to your function declaration style,
or any other aspect. If you say both/all are allowed, you quickly get to the
point of not really having a style (which may be acceptible here).
> I know if I submit a patch and the only feedback is that I should have used
> /* */ for comments, I'm extremely unlikely spend extra time to resubmit the
> patch for pedantry.
Hence my desire for the tool. Ideally, style is best enforced when its a
non-issue during the review process (i.e. a tool is able to tell you where
your style problems are, and the issue never comes up during review). We can as
you previously suggested fork a tool and modify it to conform to some other
style guidelines of our own design. But honestly, I don't want to invest alot
of time in what the guidelines are. i.e. I value consistency in style, not a
specific style. As such, borrowing checkpatch (or some other tool), and
adopting its style enforcement, seems like the best, most efficient path forward
in my mind.
Neil
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-09 19:38 ` Jay Rolette
2015-04-09 20:14 ` Neil Horman
@ 2015-04-09 21:10 ` Wiles, Keith
2015-04-09 21:23 ` Stephen Hemminger
2015-04-10 0:26 ` Neil Horman
1 sibling, 2 replies; 62+ messages in thread
From: Wiles, Keith @ 2015-04-09 21:10 UTC (permalink / raw)
To: Jay Rolette, Neil Horman; +Cc: dev
On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>
>> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
>> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
>> > stephen@networkplumber.org> wrote:
>> >
>> > > On Wed, 8 Apr 2015 16:29:54 -0600
>> > > Jay Rolette <rolette@infiniteio.com> wrote:
>> > >
>> > > > "C comments" includes //, right? It's been part of the C standard
>> for a
>> > > long time now...
>> > >
>> > > Yes but.
>> > > I like to use checkpatch and checkpatch enforces kernel style which
>> does
>> > > not allow // for
>> > > comments.
>> > >
>> >
>> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
>> > requirement to follow all of its rules
>> >
>>
>> Doesn't that beg the question, why? I understand the DPDK isn't the
>> kernel, but
>> we're not talking about clarity of code, not anything functional to that
>> code.
>> It seems we would be better served by just taking something that works
>>here
>> rather than re-inventing the wheel and digging into the minuate of what
>> type of
>> comments should be allowed (unless there is a compelling reason to
>>change
>> it
>> that supercedes the avilable tools). If not checkpath, then some other
>> tool,
>> but It seems to me that coding style is one of those things where we can
>> bend to
>> the tool rather than taking the time to make the tool do exactly whats
>> desired,
>> at least until someone gets the time to modify it.
>>
>
>Fair question.
>
>It depends a bit on how much you want to encourage patch contributions. Is
>it worth adding more pain for folks trying to contribute patches for
>things
>like this?
>
>Should we force someone to spend time redoing a patch because of which way
>they do their parenthesis? What about number of spaces to indent code? //
>vs /* */ comments? None of these matter functionally and they don't affect
>maintenance generally.
>
>If someone is modifying existing code, then yeah, they should follow the
>prevailing style (indention level, brace alignment, etc.) of the file they
>are in. It helps readability, which makes maintenance easier. However,
>IMO,
>mixing // and /* */ for comments doesn't affect the readability of the
>source.
>
>I know if I submit a patch and the only feedback is that I should have
>used
>/* */ for comments, I'm extremely unlikely spend extra time to resubmit
>the
>patch for pedantry.
I looked at checkpatch.pl for few minutes and the code does check for C99
comments and adding a command line option to allow C99 comments could
pretty simple. I found the code around line 3048 or search for C99, it is
possible it could accepted back into Linux as long as the default option
was to not allow C99 comments.
Allowing C99 comments would be nice and the only problem I could see if
some compiler has a problem with them. I believe all of the compilers we
support allow C99 comments.
The only other reason to allow them is if we add some open source code in
the future to DPDK which has C99 comments and if would be a pain to have
to convert that code every time the open source group released a new
version. It does open that path IMO.
Regards,
++Keith
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 21:10 ` Wiles, Keith
@ 2015-04-09 21:23 ` Stephen Hemminger
2015-04-09 21:29 ` Wiles, Keith
2015-04-10 0:26 ` Neil Horman
1 sibling, 1 reply; 62+ messages in thread
From: Stephen Hemminger @ 2015-04-09 21:23 UTC (permalink / raw)
To: Wiles, Keith; +Cc: dev
On Thu, 9 Apr 2015 21:10:19 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>
> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>
> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> >> > stephen@networkplumber.org> wrote:
> >> >
> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
> >> > >
> >> > > > "C comments" includes //, right? It's been part of the C standard
> >> for a
> >> > > long time now...
> >> > >
> >> > > Yes but.
> >> > > I like to use checkpatch and checkpatch enforces kernel style which
> >> does
> >> > > not allow // for
> >> > > comments.
> >> > >
> >> >
> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> >> > requirement to follow all of its rules
> >> >
> >>
> >> Doesn't that beg the question, why? I understand the DPDK isn't the
> >> kernel, but
> >> we're not talking about clarity of code, not anything functional to that
> >> code.
> >> It seems we would be better served by just taking something that works
> >>here
> >> rather than re-inventing the wheel and digging into the minuate of what
> >> type of
> >> comments should be allowed (unless there is a compelling reason to
> >>change
> >> it
> >> that supercedes the avilable tools). If not checkpath, then some other
> >> tool,
> >> but It seems to me that coding style is one of those things where we can
> >> bend to
> >> the tool rather than taking the time to make the tool do exactly whats
> >> desired,
> >> at least until someone gets the time to modify it.
> >>
> >
> >Fair question.
> >
> >It depends a bit on how much you want to encourage patch contributions. Is
> >it worth adding more pain for folks trying to contribute patches for
> >things
> >like this?
> >
> >Should we force someone to spend time redoing a patch because of which way
> >they do their parenthesis? What about number of spaces to indent code? //
> >vs /* */ comments? None of these matter functionally and they don't affect
> >maintenance generally.
> >
> >If someone is modifying existing code, then yeah, they should follow the
> >prevailing style (indention level, brace alignment, etc.) of the file they
> >are in. It helps readability, which makes maintenance easier. However,
> >IMO,
> >mixing // and /* */ for comments doesn't affect the readability of the
> >source.
> >
> >I know if I submit a patch and the only feedback is that I should have
> >used
> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
> >the
> >patch for pedantry.
>
> I looked at checkpatch.pl for few minutes and the code does check for C99
> comments and adding a command line option to allow C99 comments could
> pretty simple. I found the code around line 3048 or search for C99, it is
> possible it could accepted back into Linux as long as the default option
> was to not allow C99 comments.
>
> Allowing C99 comments would be nice and the only problem I could see if
> some compiler has a problem with them. I believe all of the compilers we
> support allow C99 comments.
>
> The only other reason to allow them is if we add some open source code in
> the future to DPDK which has C99 comments and if would be a pain to have
> to convert that code every time the open source group released a new
> version. It does open that path IMO.
>
> Regards,
> ++Keith
> >
>
But forking a tool means maintaining that tool.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 21:23 ` Stephen Hemminger
@ 2015-04-09 21:29 ` Wiles, Keith
2015-04-10 0:16 ` Neil Horman
0 siblings, 1 reply; 62+ messages in thread
From: Wiles, Keith @ 2015-04-09 21:29 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On 4/9/15, 4:23 PM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:
>On Thu, 9 Apr 2015 21:10:19 +0000
>"Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>>
>>
>> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>>
>> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
>>wrote:
>> >
>> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
>> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
>> >> > stephen@networkplumber.org> wrote:
>> >> >
>> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
>> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
>> >> > >
>> >> > > > "C comments" includes //, right? It's been part of the C
>>standard
>> >> for a
>> >> > > long time now...
>> >> > >
>> >> > > Yes but.
>> >> > > I like to use checkpatch and checkpatch enforces kernel style
>>which
>> >> does
>> >> > > not allow // for
>> >> > > comments.
>> >> > >
>> >> >
>> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
>> >> > requirement to follow all of its rules
>> >> >
>> >>
>> >> Doesn't that beg the question, why? I understand the DPDK isn't the
>> >> kernel, but
>> >> we're not talking about clarity of code, not anything functional to
>>that
>> >> code.
>> >> It seems we would be better served by just taking something that
>>works
>> >>here
>> >> rather than re-inventing the wheel and digging into the minuate of
>>what
>> >> type of
>> >> comments should be allowed (unless there is a compelling reason to
>> >>change
>> >> it
>> >> that supercedes the avilable tools). If not checkpath, then some
>>other
>> >> tool,
>> >> but It seems to me that coding style is one of those things where we
>>can
>> >> bend to
>> >> the tool rather than taking the time to make the tool do exactly
>>whats
>> >> desired,
>> >> at least until someone gets the time to modify it.
>> >>
>> >
>> >Fair question.
>> >
>> >It depends a bit on how much you want to encourage patch
>>contributions. Is
>> >it worth adding more pain for folks trying to contribute patches for
>> >things
>> >like this?
>> >
>> >Should we force someone to spend time redoing a patch because of which
>>way
>> >they do their parenthesis? What about number of spaces to indent code?
>>//
>> >vs /* */ comments? None of these matter functionally and they don't
>>affect
>> >maintenance generally.
>> >
>> >If someone is modifying existing code, then yeah, they should follow
>>the
>> >prevailing style (indention level, brace alignment, etc.) of the file
>>they
>> >are in. It helps readability, which makes maintenance easier. However,
>> >IMO,
>> >mixing // and /* */ for comments doesn't affect the readability of the
>> >source.
>> >
>> >I know if I submit a patch and the only feedback is that I should have
>> >used
>> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
>> >the
>> >patch for pedantry.
>>
>> I looked at checkpatch.pl for few minutes and the code does check for
>>C99
>> comments and adding a command line option to allow C99 comments could
>> pretty simple. I found the code around line 3048 or search for C99, it
>>is
>> possible it could accepted back into Linux as long as the default option
>> was to not allow C99 comments.
>>
>> Allowing C99 comments would be nice and the only problem I could see if
>> some compiler has a problem with them. I believe all of the compilers we
>> support allow C99 comments.
>>
>> The only other reason to allow them is if we add some open source code
>>in
>> the future to DPDK which has C99 comments and if would be a pain to have
>> to convert that code every time the open source group released a new
>> version. It does open that path IMO.
>>
>> Regards,
>> ++Keith
>> >
>>
>
>But forking a tool means maintaining that tool.
If the tool is pushed back into the main stream, then no you do not have
to maintain it, right?
That was my point and the change a simple one plus I would expect it
should not give anyone a problem unless Linux really wants to stay pre
C99, which is not the case today, right?
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 21:29 ` Wiles, Keith
@ 2015-04-10 0:16 ` Neil Horman
0 siblings, 0 replies; 62+ messages in thread
From: Neil Horman @ 2015-04-10 0:16 UTC (permalink / raw)
To: Wiles, Keith; +Cc: dev
On Thu, Apr 09, 2015 at 09:29:14PM +0000, Wiles, Keith wrote:
>
>
> On 4/9/15, 4:23 PM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:
>
> >On Thu, 9 Apr 2015 21:10:19 +0000
> >"Wiles, Keith" <keith.wiles@intel.com> wrote:
> >
> >>
> >>
> >> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
> >>
> >> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
> >>wrote:
> >> >
> >> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> >> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> >> >> > stephen@networkplumber.org> wrote:
> >> >> >
> >> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
> >> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
> >> >> > >
> >> >> > > > "C comments" includes //, right? It's been part of the C
> >>standard
> >> >> for a
> >> >> > > long time now...
> >> >> > >
> >> >> > > Yes but.
> >> >> > > I like to use checkpatch and checkpatch enforces kernel style
> >>which
> >> >> does
> >> >> > > not allow // for
> >> >> > > comments.
> >> >> > >
> >> >> >
> >> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> >> >> > requirement to follow all of its rules
> >> >> >
> >> >>
> >> >> Doesn't that beg the question, why? I understand the DPDK isn't the
> >> >> kernel, but
> >> >> we're not talking about clarity of code, not anything functional to
> >>that
> >> >> code.
> >> >> It seems we would be better served by just taking something that
> >>works
> >> >>here
> >> >> rather than re-inventing the wheel and digging into the minuate of
> >>what
> >> >> type of
> >> >> comments should be allowed (unless there is a compelling reason to
> >> >>change
> >> >> it
> >> >> that supercedes the avilable tools). If not checkpath, then some
> >>other
> >> >> tool,
> >> >> but It seems to me that coding style is one of those things where we
> >>can
> >> >> bend to
> >> >> the tool rather than taking the time to make the tool do exactly
> >>whats
> >> >> desired,
> >> >> at least until someone gets the time to modify it.
> >> >>
> >> >
> >> >Fair question.
> >> >
> >> >It depends a bit on how much you want to encourage patch
> >>contributions. Is
> >> >it worth adding more pain for folks trying to contribute patches for
> >> >things
> >> >like this?
> >> >
> >> >Should we force someone to spend time redoing a patch because of which
> >>way
> >> >they do their parenthesis? What about number of spaces to indent code?
> >>//
> >> >vs /* */ comments? None of these matter functionally and they don't
> >>affect
> >> >maintenance generally.
> >> >
> >> >If someone is modifying existing code, then yeah, they should follow
> >>the
> >> >prevailing style (indention level, brace alignment, etc.) of the file
> >>they
> >> >are in. It helps readability, which makes maintenance easier. However,
> >> >IMO,
> >> >mixing // and /* */ for comments doesn't affect the readability of the
> >> >source.
> >> >
> >> >I know if I submit a patch and the only feedback is that I should have
> >> >used
> >> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
> >> >the
> >> >patch for pedantry.
> >>
> >> I looked at checkpatch.pl for few minutes and the code does check for
> >>C99
> >> comments and adding a command line option to allow C99 comments could
> >> pretty simple. I found the code around line 3048 or search for C99, it
> >>is
> >> possible it could accepted back into Linux as long as the default option
> >> was to not allow C99 comments.
> >>
> >> Allowing C99 comments would be nice and the only problem I could see if
> >> some compiler has a problem with them. I believe all of the compilers we
> >> support allow C99 comments.
> >>
> >> The only other reason to allow them is if we add some open source code
> >>in
> >> the future to DPDK which has C99 comments and if would be a pain to have
> >> to convert that code every time the open source group released a new
> >> version. It does open that path IMO.
> >>
> >> Regards,
> >> ++Keith
> >> >
> >>
> >
> >But forking a tool means maintaining that tool.
>
> If the tool is pushed back into the main stream, then no you do not have
> to maintain it, right?
> That was my point and the change a simple one plus I would expect it
> should not give anyone a problem unless Linux really wants to stay pre
> C99, which is not the case today, right?
This is true, but the typical attitude in the linux kernel is that a change must
have a use case and a user to be considered. I don't want to dissuade you from
trying to get it accepted, but my concern would be that, since the kernel folks
don't allow the '//' comments, they won't be interested in maintaining the
change.
Neil
> >
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-09 21:10 ` Wiles, Keith
2015-04-09 21:23 ` Stephen Hemminger
@ 2015-04-10 0:26 ` Neil Horman
2015-04-10 1:49 ` Wiles, Keith
1 sibling, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-10 0:26 UTC (permalink / raw)
To: Wiles, Keith; +Cc: dev
On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith wrote:
>
>
> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>
> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> >> > stephen@networkplumber.org> wrote:
> >> >
> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
> >> > >
> >> > > > "C comments" includes //, right? It's been part of the C standard
> >> for a
> >> > > long time now...
> >> > >
> >> > > Yes but.
> >> > > I like to use checkpatch and checkpatch enforces kernel style which
> >> does
> >> > > not allow // for
> >> > > comments.
> >> > >
> >> >
> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> >> > requirement to follow all of its rules
> >> >
> >>
> >> Doesn't that beg the question, why? I understand the DPDK isn't the
> >> kernel, but
> >> we're not talking about clarity of code, not anything functional to that
> >> code.
> >> It seems we would be better served by just taking something that works
> >>here
> >> rather than re-inventing the wheel and digging into the minuate of what
> >> type of
> >> comments should be allowed (unless there is a compelling reason to
> >>change
> >> it
> >> that supercedes the avilable tools). If not checkpath, then some other
> >> tool,
> >> but It seems to me that coding style is one of those things where we can
> >> bend to
> >> the tool rather than taking the time to make the tool do exactly whats
> >> desired,
> >> at least until someone gets the time to modify it.
> >>
> >
> >Fair question.
> >
> >It depends a bit on how much you want to encourage patch contributions. Is
> >it worth adding more pain for folks trying to contribute patches for
> >things
> >like this?
> >
> >Should we force someone to spend time redoing a patch because of which way
> >they do their parenthesis? What about number of spaces to indent code? //
> >vs /* */ comments? None of these matter functionally and they don't affect
> >maintenance generally.
> >
> >If someone is modifying existing code, then yeah, they should follow the
> >prevailing style (indention level, brace alignment, etc.) of the file they
> >are in. It helps readability, which makes maintenance easier. However,
> >IMO,
> >mixing // and /* */ for comments doesn't affect the readability of the
> >source.
> >
> >I know if I submit a patch and the only feedback is that I should have
> >used
> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
> >the
> >patch for pedantry.
>
> I looked at checkpatch.pl for few minutes and the code does check for C99
> comments and adding a command line option to allow C99 comments could
> pretty simple. I found the code around line 3048 or search for C99, it is
> possible it could accepted back into Linux as long as the default option
> was to not allow C99 comments.
>
> Allowing C99 comments would be nice and the only problem I could see if
> some compiler has a problem with them. I believe all of the compilers we
> support allow C99 comments.
>
> The only other reason to allow them is if we add some open source code in
> the future to DPDK which has C99 comments and if would be a pain to have
> to convert that code every time the open source group released a new
> version. It does open that path IMO.
>
So, this again seems to be bad philosophy in my mind. If we are, to use your
exmple, accept code into the DPDK in the future with comments that violate our
selected style, it is then, by definition, in violation of the style guidelines.
If we accept it anyway, or if we allow both styles (by documenting it/codifying
it a tool to check for/etc) then we dilute the style guide. Maybe in some
cases, such as this, thats ok, but its something to be cogniscent of.
Especially if making the choice to allow both put us in a position of having to
maintain a tool to do the checking, then I think we need to fall on the side of
going with what the tool (checkpatch or something else) does, unless we have a
maintainer stepping up.
The bottom line is that style guides enforce style, and tooling makes
contributors condusive to following the style. If we have someone that is
willing to maintain such a tool, then we have a lot of leway in what the style
is, but if we don't then we really need to follow the style that an existing
tool provides, because without tooling, contributors aren't likely to bother
with strict adherence to the style.
Neil
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-10 0:26 ` Neil Horman
@ 2015-04-10 1:49 ` Wiles, Keith
2015-04-10 11:41 ` Neil Horman
0 siblings, 1 reply; 62+ messages in thread
From: Wiles, Keith @ 2015-04-10 1:49 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
On 4/9/15, 7:26 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
>On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith wrote:
>>
>>
>> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>>
>> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
>>wrote:
>> >
>> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
>> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
>> >> > stephen@networkplumber.org> wrote:
>> >> >
>> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
>> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
>> >> > >
>> >> > > > "C comments" includes //, right? It's been part of the C
>>standard
>> >> for a
>> >> > > long time now...
>> >> > >
>> >> > > Yes but.
>> >> > > I like to use checkpatch and checkpatch enforces kernel style
>>which
>> >> does
>> >> > > not allow // for
>> >> > > comments.
>> >> > >
>> >> >
>> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
>> >> > requirement to follow all of its rules
>> >> >
>> >>
>> >> Doesn't that beg the question, why? I understand the DPDK isn't the
>> >> kernel, but
>> >> we're not talking about clarity of code, not anything functional to
>>that
>> >> code.
>> >> It seems we would be better served by just taking something that
>>works
>> >>here
>> >> rather than re-inventing the wheel and digging into the minuate of
>>what
>> >> type of
>> >> comments should be allowed (unless there is a compelling reason to
>> >>change
>> >> it
>> >> that supercedes the avilable tools). If not checkpath, then some
>>other
>> >> tool,
>> >> but It seems to me that coding style is one of those things where we
>>can
>> >> bend to
>> >> the tool rather than taking the time to make the tool do exactly
>>whats
>> >> desired,
>> >> at least until someone gets the time to modify it.
>> >>
>> >
>> >Fair question.
>> >
>> >It depends a bit on how much you want to encourage patch
>>contributions. Is
>> >it worth adding more pain for folks trying to contribute patches for
>> >things
>> >like this?
>> >
>> >Should we force someone to spend time redoing a patch because of which
>>way
>> >they do their parenthesis? What about number of spaces to indent code?
>>//
>> >vs /* */ comments? None of these matter functionally and they don't
>>affect
>> >maintenance generally.
>> >
>> >If someone is modifying existing code, then yeah, they should follow
>>the
>> >prevailing style (indention level, brace alignment, etc.) of the file
>>they
>> >are in. It helps readability, which makes maintenance easier. However,
>> >IMO,
>> >mixing // and /* */ for comments doesn't affect the readability of the
>> >source.
>> >
>> >I know if I submit a patch and the only feedback is that I should have
>> >used
>> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
>> >the
>> >patch for pedantry.
>>
>> I looked at checkpatch.pl for few minutes and the code does check for
>>C99
>> comments and adding a command line option to allow C99 comments could
>> pretty simple. I found the code around line 3048 or search for C99, it
>>is
>> possible it could accepted back into Linux as long as the default option
>> was to not allow C99 comments.
>>
>> Allowing C99 comments would be nice and the only problem I could see if
>> some compiler has a problem with them. I believe all of the compilers we
>> support allow C99 comments.
>>
>> The only other reason to allow them is if we add some open source code
>>in
>> the future to DPDK which has C99 comments and if would be a pain to have
>> to convert that code every time the open source group released a new
>> version. It does open that path IMO.
>>
>
>So, this again seems to be bad philosophy in my mind. If we are, to use
>your
>exmple, accept code into the DPDK in the future with comments that
>violate our
>selected style, it is then, by definition, in violation of the style
>guidelines.
Who stated it violated the style guidelines, we do not have a style
guideline yet. This why Shiobhan sent out the guidelines in the first
place.
>If we accept it anyway, or if we allow both styles (by documenting
>it/codifying
>it a tool to check for/etc) then we dilute the style guide. Maybe in some
>cases, such as this, thats ok, but its something to be cogniscent of.
>Especially if making the choice to allow both put us in a position of
>having to
>maintain a tool to do the checking, then I think we need to fall on the
>side of
>going with what the tool (checkpatch or something else) does, unless we
>have a
>maintainer stepping up.
Sure maintaining a tool for the C99 comment seems pretty easy, if no one
want to maintain the tool that is another problem. Whether to support C99
comments is the point not that we have to maintain the tool. At this point
checkpatch is a OK tool if you only want Linux kernel coding style and at
this point we are not Linux code style. It just so happens the checkpatch
provides some checks we seem to agree with.
If we really want some type of tool to check every detail of Œour¹ coding
style then we most likely need to stop using checkpatch IMO for a tool
like astyle, ununcrustify or some tool that provides the best solution.
I was suggesting we could use one of these tool to create a common config
file for everyone to use and then checkpatch may not be required. At least
these other tools beside check patch seem to have a huge number of options
to format the automatically instead of the developer having to do that
work by hand.
>
>The bottom line is that style guides enforce style, and tooling makes
>contributors condusive to following the style. If we have someone that is
>willing to maintain such a tool, then we have a lot of leway in what the
>style
>is, but if we don't then we really need to follow the style that an
>existing
>tool provides, because without tooling, contributors aren't likely to
>bother
>with strict adherence to the style.
For tools like astyle or uncrustify we do not have to maintain those tools
as they are already being maintained, we just have to use them. It seems a
waste to me we are not willing to change because someone may have to make
a single change to checkpatch, which we may never have to change again
buying we can not get it upstreamed. It is possible we can drop the use of
checkpatch and use one of the other tools made just for style formatting.
To me checkpatch is a tool that needs to be replaced by tools that are
made for code formatting, not just checking. I would assume checkpatch has
its place in the Linux kernel and it needs to be used only by the Linux
Kernel. I believe it we took the time to format the code to one of the
tools and require a developer to use that tool with the provided config
file it would be simpler to enforce the coding style.
As we patch a file the tool could be applied before the commit. It will
effect other patches to that file, but we have to do merges today and it
will not change anytime soon. If you applied a tool to format all of the
code at some flag day, we would have a bit of churn then it would be over.
I would suggest we focus on the coding style and see what it means to the
tool/checkpatch. Read the email from Siobhan and then we can talk about
the coding style in more meaning full detail.
Keith
>
>Neil
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-10 1:49 ` Wiles, Keith
@ 2015-04-10 11:41 ` Neil Horman
2015-04-10 14:43 ` Wiles, Keith
0 siblings, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-10 11:41 UTC (permalink / raw)
To: Wiles, Keith; +Cc: dev
On Fri, Apr 10, 2015 at 01:49:33AM +0000, Wiles, Keith wrote:
>
>
> On 4/9/15, 7:26 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
>
> >On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith wrote:
> >>
> >>
> >> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
> >>
> >> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
> >>wrote:
> >> >
> >> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
> >> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
> >> >> > stephen@networkplumber.org> wrote:
> >> >> >
> >> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
> >> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
> >> >> > >
> >> >> > > > "C comments" includes //, right? It's been part of the C
> >>standard
> >> >> for a
> >> >> > > long time now...
> >> >> > >
> >> >> > > Yes but.
> >> >> > > I like to use checkpatch and checkpatch enforces kernel style
> >>which
> >> >> does
> >> >> > > not allow // for
> >> >> > > comments.
> >> >> > >
> >> >> >
> >> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so no
> >> >> > requirement to follow all of its rules
> >> >> >
> >> >>
> >> >> Doesn't that beg the question, why? I understand the DPDK isn't the
> >> >> kernel, but
> >> >> we're not talking about clarity of code, not anything functional to
> >>that
> >> >> code.
> >> >> It seems we would be better served by just taking something that
> >>works
> >> >>here
> >> >> rather than re-inventing the wheel and digging into the minuate of
> >>what
> >> >> type of
> >> >> comments should be allowed (unless there is a compelling reason to
> >> >>change
> >> >> it
> >> >> that supercedes the avilable tools). If not checkpath, then some
> >>other
> >> >> tool,
> >> >> but It seems to me that coding style is one of those things where we
> >>can
> >> >> bend to
> >> >> the tool rather than taking the time to make the tool do exactly
> >>whats
> >> >> desired,
> >> >> at least until someone gets the time to modify it.
> >> >>
> >> >
> >> >Fair question.
> >> >
> >> >It depends a bit on how much you want to encourage patch
> >>contributions. Is
> >> >it worth adding more pain for folks trying to contribute patches for
> >> >things
> >> >like this?
> >> >
> >> >Should we force someone to spend time redoing a patch because of which
> >>way
> >> >they do their parenthesis? What about number of spaces to indent code?
> >>//
> >> >vs /* */ comments? None of these matter functionally and they don't
> >>affect
> >> >maintenance generally.
> >> >
> >> >If someone is modifying existing code, then yeah, they should follow
> >>the
> >> >prevailing style (indention level, brace alignment, etc.) of the file
> >>they
> >> >are in. It helps readability, which makes maintenance easier. However,
> >> >IMO,
> >> >mixing // and /* */ for comments doesn't affect the readability of the
> >> >source.
> >> >
> >> >I know if I submit a patch and the only feedback is that I should have
> >> >used
> >> >/* */ for comments, I'm extremely unlikely spend extra time to resubmit
> >> >the
> >> >patch for pedantry.
> >>
> >> I looked at checkpatch.pl for few minutes and the code does check for
> >>C99
> >> comments and adding a command line option to allow C99 comments could
> >> pretty simple. I found the code around line 3048 or search for C99, it
> >>is
> >> possible it could accepted back into Linux as long as the default option
> >> was to not allow C99 comments.
> >>
> >> Allowing C99 comments would be nice and the only problem I could see if
> >> some compiler has a problem with them. I believe all of the compilers we
> >> support allow C99 comments.
> >>
> >> The only other reason to allow them is if we add some open source code
> >>in
> >> the future to DPDK which has C99 comments and if would be a pain to have
> >> to convert that code every time the open source group released a new
> >> version. It does open that path IMO.
> >>
> >
> >So, this again seems to be bad philosophy in my mind. If we are, to use
> >your
> >exmple, accept code into the DPDK in the future with comments that
> >violate our
> >selected style, it is then, by definition, in violation of the style
> >guidelines.
>
> Who stated it violated the style guidelines, we do not have a style
> guideline yet. This why Shiobhan sent out the guidelines in the first
> place.
>
No one said anyone is violating our style guidelines, I think thats pretty
obvious. You had said that allowing C99 comments would be a nice thing to be
able to do in the future if we were to want to integrate a submission that had
them. While thats true, I was pointing out that what you were suggesting was
asserting the notion that doing so was relaxing style guidelines to allow for
easier acceptance, while style guidelines (as a general notion, not as any
specific set of rules we currently do or may in the future assert) are there to
enforce a consistent look in the code.
Put another way, we can certainly allow for both styles of comment (or multiple
styles of any aspect of the code), if we want to, but doing so is somewhat
against the notion of a 'style', as a style provides a consistent look, and
making the argument that we allow both because it makes for easier code
submission and acceptance is a bit backwards.
Note, I'm not saying that we can't accept both types of comments (or multiple
types of any kind of style aspect), only that, if we are to consider multiple
types of any style aspect, we should decide based on how it will make the code
look, not on how much harder it will be to get the code accepted during review.
1) If code acceptance is the goal, we should have no style guideline
2) If pretty, easy to read code is the goal, we should make the style fairly
spicific with very limited options for style choices
If the goal is a happy medium, we should make sure that we have a tool that
makes (2) easy to achive so that at review time on the list, style is a
non-issue.
> >If we accept it anyway, or if we allow both styles (by documenting
> >it/codifying
> >it a tool to check for/etc) then we dilute the style guide. Maybe in some
> >cases, such as this, thats ok, but its something to be cogniscent of.
> >Especially if making the choice to allow both put us in a position of
> >having to
> >maintain a tool to do the checking, then I think we need to fall on the
> >side of
> >going with what the tool (checkpatch or something else) does, unless we
> >have a
> >maintainer stepping up.
>
> Sure maintaining a tool for the C99 comment seems pretty easy, if no one
> want to maintain the tool that is another problem.
If this is you stepping up for that role, then we're good to go, that means we
can have a tool that supports any style we want to implement.
> Whether to support C99
> comments is the point not that we have to maintain the tool. At this point
> checkpatch is a OK tool if you only want Linux kernel coding style and at
> this point we are not Linux code style. It just so happens the checkpatch
> provides some checks we seem to agree with.
>
You're right here, we should be deciding if we want to support C99 comments or
not. But so far doing so has the implication of needing to
maintain/aquire/write a tool to do so. If some other tool allows for the style
we choose, so be it. But so far it seems like we havent found a tool that does
that in a way we are generally happy with, and so the question of how to enforce
the style has remained hanging out there. But yes, if we have a tool that can
be configured for this, we can stop talking about the tooling aspect.
> If we really want some type of tool to check every detail of Œour¹ coding
> style then we most likely need to stop using checkpatch IMO for a tool
> like astyle, ununcrustify or some tool that provides the best solution.
>
Yes, we do, we have to, otherwise style never gets enforced. Though astyle and
uncrusity are beautifiers, not checkers. While I'm not opposed to a tool that
formats the code for you, I think we need to require that it be able to operate
in a dry-run mode, where it simply points out your formatting deviations, so
that you can make directions, or decide you need to violate the guidelines.
> I was suggesting we could use one of these tool to create a common config
> file for everyone to use and then checkpatch may not be required. At least
> these other tools beside check patch seem to have a huge number of options
> to format the automatically instead of the developer having to do that
> work by hand.
Thats fine, as long as it has a mode whereby it just tells you whats wrong, and
doesn't fix it for you.
> >
> >The bottom line is that style guides enforce style, and tooling makes
> >contributors condusive to following the style. If we have someone that is
> >willing to maintain such a tool, then we have a lot of leway in what the
> >style
> >is, but if we don't then we really need to follow the style that an
> >existing
> >tool provides, because without tooling, contributors aren't likely to
> >bother
> >with strict adherence to the style.
>
> For tools like astyle or uncrustify we do not have to maintain those tools
> as they are already being maintained, we just have to use them. It seems a
> waste to me we are not willing to change because someone may have to make
> a single change to checkpatch, which we may never have to change again
> buying we can not get it upstreamed. It is possible we can drop the use of
> checkpatch and use one of the other tools made just for style formatting.
>
I'm not opposed to making a change to checkpatch, we just need to know whos
going to do it and maintain it if Linus doesn't accept it. If thats you, or you
know someone who will step up, we're good to go.
> To me checkpatch is a tool that needs to be replaced by tools that are
> made for code formatting, not just checking. I would assume checkpatch has
> its place in the Linux kernel and it needs to be used only by the Linux
> Kernel.
Thats right, but theres no reason we can't adopt it. Not that we have to, but
it seems like we already have lots of code that follows its style.
> I believe it we took the time to format the code to one of the
> tools and require a developer to use that tool with the provided config
> file it would be simpler to enforce the coding style.
>
Question: What about the kernel code that we borrow from the upstream projects
(igb/e1000/ixgbe kernel code)? Are you proposing an automatic reformatting
there? If we have expectations of integrating future kernel code, it might be
worthwhile adopting the kernel style (at least for those files). I realize
thats exactly the argument I made in opposition above (that we shouldn't select
style based on code submissions), but I wanted to point it out. Or are we
exempting kernel code from this?
> As we patch a file the tool could be applied before the commit. It will
> effect other patches to that file, but we have to do merges today and it
> will not change anytime soon. If you applied a tool to format all of the
> code at some flag day, we would have a bit of churn then it would be over.
>
This is true, as noted above, I'm fine with some other tool, but I would like to
see it have an informational mode, where it just points out errors in style
rather than automatically correcting them.
> I would suggest we focus on the coding style and see what it means to the
> tool/checkpatch. Read the email from Siobhan and then we can talk about
> the coding style in more meaning full detail.
>
> Keith
> >
> >Neil
> >
> >
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-10 11:41 ` Neil Horman
@ 2015-04-10 14:43 ` Wiles, Keith
0 siblings, 0 replies; 62+ messages in thread
From: Wiles, Keith @ 2015-04-10 14:43 UTC (permalink / raw)
To: Neil Horman; +Cc: dev
On 4/10/15, 6:41 AM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
>On Fri, Apr 10, 2015 at 01:49:33AM +0000, Wiles, Keith wrote:
>>
>>
>> On 4/9/15, 7:26 PM, "Neil Horman" <nhorman@tuxdriver.com> wrote:
>>
>> >On Thu, Apr 09, 2015 at 09:10:19PM +0000, Wiles, Keith wrote:
>> >>
>> >>
>> >> On 4/9/15, 2:38 PM, "Jay Rolette" <rolette@infiniteio.com> wrote:
>> >>
>> >> >On Thu, Apr 9, 2015 at 2:16 PM, Neil Horman <nhorman@tuxdriver.com>
>> >>wrote:
>> >> >
>> >> >> On Thu, Apr 09, 2015 at 11:31:39AM -0500, Jay Rolette wrote:
>> >> >> > On Wed, Apr 8, 2015 at 5:38 PM, Stephen Hemminger <
>> >> >> > stephen@networkplumber.org> wrote:
>> >> >> >
>> >> >> > > On Wed, 8 Apr 2015 16:29:54 -0600
>> >> >> > > Jay Rolette <rolette@infiniteio.com> wrote:
>> >> >> > >
>> >> >> > > > "C comments" includes //, right? It's been part of the C
>> >>standard
>> >> >> for a
>> >> >> > > long time now...
>> >> >> > >
>> >> >> > > Yes but.
>> >> >> > > I like to use checkpatch and checkpatch enforces kernel style
>> >>which
>> >> >> does
>> >> >> > > not allow // for
>> >> >> > > comments.
>> >> >> > >
>> >> >> >
>> >> >> > Fork checkpatch and disable that bit? DPDK isn't the kernel, so
>>no
>> >> >> > requirement to follow all of its rules
>> >> >> >
>> >> >>
>> >> >> Doesn't that beg the question, why? I understand the DPDK isn't
>>the
>> >> >> kernel, but
>> >> >> we're not talking about clarity of code, not anything functional
>>to
>> >>that
>> >> >> code.
>> >> >> It seems we would be better served by just taking something that
>> >>works
>> >> >>here
>> >> >> rather than re-inventing the wheel and digging into the minuate of
>> >>what
>> >> >> type of
>> >> >> comments should be allowed (unless there is a compelling reason to
>> >> >>change
>> >> >> it
>> >> >> that supercedes the avilable tools). If not checkpath, then some
>> >>other
>> >> >> tool,
>> >> >> but It seems to me that coding style is one of those things where
>>we
>> >>can
>> >> >> bend to
>> >> >> the tool rather than taking the time to make the tool do exactly
>> >>whats
>> >> >> desired,
>> >> >> at least until someone gets the time to modify it.
>> >> >>
>> >> >
>> >> >Fair question.
>> >> >
>> >> >It depends a bit on how much you want to encourage patch
>> >>contributions. Is
>> >> >it worth adding more pain for folks trying to contribute patches for
>> >> >things
>> >> >like this?
>> >> >
>> >> >Should we force someone to spend time redoing a patch because of
>>which
>> >>way
>> >> >they do their parenthesis? What about number of spaces to indent
>>code?
>> >>//
>> >> >vs /* */ comments? None of these matter functionally and they don't
>> >>affect
>> >> >maintenance generally.
>> >> >
>> >> >If someone is modifying existing code, then yeah, they should follow
>> >>the
>> >> >prevailing style (indention level, brace alignment, etc.) of the
>>file
>> >>they
>> >> >are in. It helps readability, which makes maintenance easier.
>>However,
>> >> >IMO,
>> >> >mixing // and /* */ for comments doesn't affect the readability of
>>the
>> >> >source.
>> >> >
>> >> >I know if I submit a patch and the only feedback is that I should
>>have
>> >> >used
>> >> >/* */ for comments, I'm extremely unlikely spend extra time to
>>resubmit
>> >> >the
>> >> >patch for pedantry.
>> >>
>> >> I looked at checkpatch.pl for few minutes and the code does check for
>> >>C99
>> >> comments and adding a command line option to allow C99 comments could
>> >> pretty simple. I found the code around line 3048 or search for C99,
>>it
>> >>is
>> >> possible it could accepted back into Linux as long as the default
>>option
>> >> was to not allow C99 comments.
>> >>
>> >> Allowing C99 comments would be nice and the only problem I could see
>>if
>> >> some compiler has a problem with them. I believe all of the
>>compilers we
>> >> support allow C99 comments.
>> >>
>> >> The only other reason to allow them is if we add some open source
>>code
>> >>in
>> >> the future to DPDK which has C99 comments and if would be a pain to
>>have
>> >> to convert that code every time the open source group released a new
>> >> version. It does open that path IMO.
>> >>
>> >
>> >So, this again seems to be bad philosophy in my mind. If we are, to
>>use
>> >your
>> >exmple, accept code into the DPDK in the future with comments that
>> >violate our
>> >selected style, it is then, by definition, in violation of the style
>> >guidelines.
>>
>> Who stated it violated the style guidelines, we do not have a style
>> guideline yet. This why Shiobhan sent out the guidelines in the first
>> place.
>>
>No one said anyone is violating our style guidelines, I think thats pretty
>obvious. You had said that allowing C99 comments would be a nice thing
>to be
>able to do in the future if we were to want to integrate a submission
>that had
>them. While thats true, I was pointing out that what you were suggesting
>was
>asserting the notion that doing so was relaxing style guidelines to allow
>for
>easier acceptance, while style guidelines (as a general notion, not as any
>specific set of rules we currently do or may in the future assert) are
>there to
>enforce a consistent look in the code.
>
>Put another way, we can certainly allow for both styles of comment (or
>multiple
>styles of any aspect of the code), if we want to, but doing so is somewhat
>against the notion of a 'style', as a style provides a consistent look,
>and
>making the argument that we allow both because it makes for easier code
>submission and acceptance is a bit backwards.
>
>Note, I'm not saying that we can't accept both types of comments (or
>multiple
>types of any kind of style aspect), only that, if we are to consider
>multiple
>types of any style aspect, we should decide based on how it will make the
>code
>look, not on how much harder it will be to get the code accepted during
>review.
>
>1) If code acceptance is the goal, we should have no style guideline
>
>2) If pretty, easy to read code is the goal, we should make the style
>fairly
>spicific with very limited options for style choices
>
>If the goal is a happy medium, we should make sure that we have a tool
>that
>makes (2) easy to achive so that at review time on the list, style is a
>non-issue.
>
>> >If we accept it anyway, or if we allow both styles (by documenting
>> >it/codifying
>> >it a tool to check for/etc) then we dilute the style guide. Maybe in
>>some
>> >cases, such as this, thats ok, but its something to be cogniscent of.
>> >Especially if making the choice to allow both put us in a position of
>> >having to
>> >maintain a tool to do the checking, then I think we need to fall on the
>> >side of
>> >going with what the tool (checkpatch or something else) does, unless we
>> >have a
>> >maintainer stepping up.
>>
>> Sure maintaining a tool for the C99 comment seems pretty easy, if no one
>> want to maintain the tool that is another problem.
>If this is you stepping up for that role, then we're good to go, that
>means we
>can have a tool that supports any style we want to implement.
>
>> Whether to support C99
>> comments is the point not that we have to maintain the tool. At this
>>point
>> checkpatch is a OK tool if you only want Linux kernel coding style and
>>at
>> this point we are not Linux code style. It just so happens the
>>checkpatch
>> provides some checks we seem to agree with.
>>
>You're right here, we should be deciding if we want to support C99
>comments or
>not. But so far doing so has the implication of needing to
>maintain/aquire/write a tool to do so. If some other tool allows for the
>style
>we choose, so be it. But so far it seems like we havent found a tool
>that does
>that in a way we are generally happy with, and so the question of how to
>enforce
>the style has remained hanging out there. But yes, if we have a tool
>that can
>be configured for this, we can stop talking about the tooling aspect.
>
>> If we really want some type of tool to check every detail of Œour¹
>>coding
>> style then we most likely need to stop using checkpatch IMO for a tool
>> like astyle, ununcrustify or some tool that provides the best solution.
>>
>Yes, we do, we have to, otherwise style never gets enforced. Though
>astyle and
>uncrusity are beautifiers, not checkers. While I'm not opposed to a tool
>that
>formats the code for you, I think we need to require that it be able to
>operate
>in a dry-run mode, where it simply points out your formatting deviations,
>so
>that you can make directions, or decide you need to violate the
>guidelines.
>
>> I was suggesting we could use one of these tool to create a common
>>config
>> file for everyone to use and then checkpatch may not be required. At
>>least
>> these other tools beside check patch seem to have a huge number of
>>options
>> to format the automatically instead of the developer having to do that
>> work by hand.
>Thats fine, as long as it has a mode whereby it just tells you whats
>wrong, and
>doesn't fix it for you.
>
>> >
>> >The bottom line is that style guides enforce style, and tooling makes
>> >contributors condusive to following the style. If we have someone
>>that is
>> >willing to maintain such a tool, then we have a lot of leway in what
>>the
>> >style
>> >is, but if we don't then we really need to follow the style that an
>> >existing
>> >tool provides, because without tooling, contributors aren't likely to
>> >bother
>> >with strict adherence to the style.
>>
>> For tools like astyle or uncrustify we do not have to maintain those
>>tools
>> as they are already being maintained, we just have to use them. It
>>seems a
>> waste to me we are not willing to change because someone may have to
>>make
>> a single change to checkpatch, which we may never have to change again
>> buying we can not get it upstreamed. It is possible we can drop the use
>>of
>> checkpatch and use one of the other tools made just for style
>>formatting.
>>
>I'm not opposed to making a change to checkpatch, we just need to know
>whos
>going to do it and maintain it if Linus doesn't accept it. If thats you,
>or you
>know someone who will step up, we're good to go.
>
>> To me checkpatch is a tool that needs to be replaced by tools that are
>> made for code formatting, not just checking. I would assume checkpatch
>>has
>> its place in the Linux kernel and it needs to be used only by the Linux
>> Kernel.
>Thats right, but theres no reason we can't adopt it. Not that we have
>to, but
>it seems like we already have lots of code that follows its style.
>
>> I believe it we took the time to format the code to one of the
>> tools and require a developer to use that tool with the provided config
>> file it would be simpler to enforce the coding style.
>>
>Question: What about the kernel code that we borrow from the upstream
>projects
>(igb/e1000/ixgbe kernel code)? Are you proposing an automatic
>reformatting
>there? If we have expectations of integrating future kernel code, it
>might be
>worthwhile adopting the kernel style (at least for those files). I
>realize
>thats exactly the argument I made in opposition above (that we shouldn't
>select
>style based on code submissions), but I wanted to point it out. Or are we
>exempting kernel code from this?
>
>
>> As we patch a file the tool could be applied before the commit. It will
>> effect other patches to that file, but we have to do merges today and it
>> will not change anytime soon. If you applied a tool to format all of the
>> code at some flag day, we would have a bit of churn then it would be
>>over.
>>
>This is true, as noted above, I'm fine with some other tool, but I would
>like to
>see it have an informational mode, where it just points out errors in
>style
>rather than automatically correcting them.
We both know a coding style is needed, what the coding style is like needs
to be defined and we need to focus on Siobhan document instead of
discussing the these distracting issues. We both seem to want a tool, lets
discuss it after we define the coding style.
Keith
>
>> I would suggest we focus on the coding style and see what it means to
>>the
>> tool/checkpatch. Read the email from Siobhan and then we can talk about
>> the coding style in more meaning full detail.
>>
>> Keith
>> >
>> >Neil
>> >
>> >
>>
>>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
2015-04-08 11:43 ` Neil Horman
@ 2015-04-08 14:16 ` Wiles, Keith
2015-04-14 14:50 ` Bruce Richardson
2015-04-08 15:21 ` Wiles, Keith
` (5 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Wiles, Keith @ 2015-04-08 14:16 UTC (permalink / raw)
To: Butler, Siobhan A, Thomas Monjalon, dev
On 4/8/15, 5:43 AM, "Butler, Siobhan A" <siobhan.a.butler@intel.com> wrote:
>Hi all,
>To add to the tools brainstorming - I propose we use the following Coding
>Standards as the basis of guidelines on coding style going forward.
>The style outlined below is in alignment with the current convention used
>for the majority of the project.
>Any thoughts/suggestions or feedback welcome.
>Thanks
>Siobhan :)
><siobhan.a.butler@intel.com>
>
>
>
>Coding Style
>~~~~~~~~~~
>
>Description
>-----------
>
>This document specifies the preferred style for source files in the DPDK
>source tree.
>It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
>Kernel Developer's Manual (see man style(9)),
>but was heavily modified for the needs of the DPDK. Many of the style
>rules are implicit in the examples.
>Be careful to check the examples before assuming that style is silent on
>an issue.
>
>General Guidelines
>------------------
>
>The rules and guidelines given in this document cannot cover every
>situation, so the following general guidelines should be used as a
>fallback:
>The code style should be consistent within each individual file, and
>within each file in a given directory or module - in the case of creating
>new files
>The primary reason for coding standards is to increase code readability
>and comprehensibility, therefore always use whatever option will make the
>code easiest to read.
>
>The following more specific recommendations apply to all sections, both
>for C and assembly code:
>Line length is recommended to be not more than 80 characters, including
>comments. [Tab stop size should be assumed to be at least 4-characters
>wide]
>Indentation should be to no more than 3 levels deep.
>NOTE The above are recommendations, and not hard limits. However, it is
>expected that the recommendations should be followed in all but the
>rarest situations.
>C Comment Style
>
>Usual Comments
>--------------
>
>These comments should be used in normal cases. To document a public API,
>a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
Did you mean to have the Œ*¹ aligned, if so good, if not then it does not
make sense to not align them. The indent of one space here does not help
convey any information IMO.
>
>License Header
>--------------
>
>Each file should begin with a special comment tag which will contain the
>appropriate copyright and license for the file (Generally BSD License).
>After any copyright header, a blank line should be left before any other
>contents, e.g. include statements in a C file.
>
>C Preprocessor Directives
>-------------------------
>
>Header Includes
>
>In DPDK sources, the include files should be ordered as following:
> libc includes (system includes first)
> DPDK EAL includes
> DPDK misc libraries includes
> application-specific includes
>
>Example:
> #include <stdio.h>
> #include <stdlib.h>
>
> #include <rte_eal.h>
>
> #include <rte_ring.h>
> #include <rte_mempool.h>
>
> #include "application.h"
>
>
>Global pathnames are defined in <paths.h>. Pathnames local to the program
>go in "pathnames.h" in the local directory.
> #include <paths.h>
>
>
>Leave another blank line before the user include files.
> #include "pathnames.h" /* Local includes in double quotes. */
>
>NOTE Please avoid, as much as possible, including headers from other
>headers file. Doing so should be properly explained and justified.
>Headers should be protected against multiple inclusion with the usual:
> #ifndef _FILE_H_
> #define _FILE_H_
>
> /* Code */
>
> #endif /* _FILE_H_ */
>
>
>Macros
>
>Do not ``#define`` or declare names in the implementation namespace
>except for implementing application interfaces.
>
>The names of ``unsafe`` macros (ones that have side effects), and the
>names of macros for manifest constants, are all in uppercase.
>
>The expansions of expression-like macros are either a single token or
>have outer parentheses. If a macro is an inline expansion of a function,
>the function name is all in lowercase and the macro has the same name all
>in uppercase. Right-justify the backslashes;
>it makes it easier to read. If the macro encapsulates a compound
>statement, enclose it in a do loop, so that it can be used safely in if
>statements.
>Any final statement-terminating semicolon should be supplied by the macro
>invocation rather than the macro, to make parsing easier for
>pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
I have seen this Œwhile( /*CONSTCOND*/0 )¹ (I think that is the comment)
plus I have see this Œwhile((0))¹ as gcc complained about a constant in
the while(). This one maybe fixed at this point in the compilers.
>
>NOTE Wherever possible, enums and typedefs should be preferred to macros,
>since they provide additional degrees
>of type-safety and can allow compilers to emit extra warnings about
>unsafe code.
>
>Conditional Compilation
>-----------------------
>
>When code is conditionally compiled using #ifdef or #if, a comment may be
>added following the matching #endif or #else to
>permit the reader to easily discern where conditionally compiled code
>regions end. This comment should be used only for
>(subjectively) long regions, regions greater than 20 lines, or where a
>series of nested #ifdef 's may be confusing to the reader.
>Exceptions may be made for cases where code is conditionally not compiled
>for the purposes of lint(1), even though the uncompiled
>region may be small. The comment should be separated from the #endif or
>#else by a single space. For short conditionally compiled regions,
>a closing comment should not be used.
>
>The comment for #endif should match the expression used in the
>corresponding #if or #ifdef. The comment for #else and #elif
>should match the inverse of the expression(s) used in the preceding #if
>and/or #elif statements. In the comments,
>the subexpression defined(FOO) is abbreviated as FOO. For the purposes of
>comments, #ifndef FOO is treated as #if !defined(FOO).
> #ifdef KTRACE
> #include <sys/ktrace.h>
> #endif
>
> #ifdef COMPAT_43
> /* A large region here, or other conditional code. */
> #else /* !COMPAT_43 */
> /* Or here. */
> #endif /* COMPAT_43 */
>
> #ifndef COMPAT_43
> /* Yet another large region here, or other conditional code. */
> #else /* COMPAT_43 */
> /* Or here. */
> #endif /* !COMPAT_43 */
>
>NOTE Conditional compilation should be used only when absolutely
>necessary, as it increases the number of target binaries that need to be
>built and tested.
>C Types
>
>Integers
>
>For fixed/minimum-size integer values, the project uses the form uintXX_t
>(from stdint.h) instead of older BSD-style integer identifiers of the
>form u_intXX_t.
>
>Enumerations
>------------
>
>Enumeration values are all uppercase.
> enum enumtype { ONE, TWO } et;
>
>
>Bitfields
>---------
>
>The developer should group bitfields that are included in the same
>integer, as follows:
> struct grehdr {
> uint16_t rec:3,
> srr:1,
> seq:1,
> key:1,
> routing:1,
> csum:1,
> version:3,
> reserved:4,
> ack:1;
> /* ... */
> }
I like to line up the Œ:¹ just to make it a bit more readable, but I can
live without it.
>
>
>Variable Declarations
>---------------------
>
>In declarations, do not put any whitespace between asterisks and adjacent
>tokens, except for tokens that are identifiers related to types.
Much more readable to have the space between pointer and token IMO.
>
>(These identifiers are the names of basic types, type qualifiers, and
>typedef-names other than the one being declared.)
>Separate these identifiers from asterisks using a single space.
Not sure I understand here, can you give an example?
>
>
>Structure Declarations
>
>When declaring variables in structures, declare them sorted by use, then
>by size (largest to smallest), and then in alphabetical order.
>Alignment constraints may override the previous rules. The first category
>normally does not apply, but there are exceptions.
>Each structure element gets its own line. Try to make the structure
>readable by aligning the member names using spaces as shown below.
>Names following extremely long types, which therefore cannot be easily
>aligned with the rest, should be separated by a single space.
Sorting variables in structures and other places just seem to much
trouble. Sometimes you want to group variables together as they are used
together in the code below.
Ordering structure members is not going work as alignment and grouping is
more important. Alignment is important as we all know holes in structures
which can cause some very odd bugs from arch to arch. Also order of the
members to a structure could be dependent on hardware or some other
overlay structure being used to reference that structure. The comments
should point this issue out in case someone decides to change the order.
My main concern is trying to keep up with fact a set of members in one
structure are sorted and in another they are not for some reason.
Sorting local variables is also not reasonable, for grouping of variables
and being able to maintain the order.
>
> struct foo {
> struct foo *next; /* List of active foo. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
>
>
>Major structures should be declared at the top of the file in which they
>are used, or in separate header files if they are used
>in multiple source files. Use of the structures should be by separate
>declarations and should be extern if they are declared in a header file.
>
>Queues
>
>Use queue(3) macros rather than rolling your own lists, whenever
>possible. Thus, the previous example would be better written:
> #include <sys/queue.h>
>
> struct foo {
> LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
> LIST_HEAD(, foo) foohead; /* Head of global foo list. */
>
>
>DPDK also provides an optimized way to store elements in lockless rings.
>This should be used in all data-path code, when there are several
>consumer and/or producers to avoid locking for concurrent access.
>
>Typedefs
>
>Avoid using typedefs for structure types. For example, use:
> struct my_struct_type {
> /* ... */
> };
>
> struct my_struct_type my_var;
>
>
>rather than:
> typedef struct my_struct_type {
> /* ... */
> } my_struct_type;
>
> my_struct_type my_var
>
>
>Typedefs are problematic because they do not properly hide their
>underlying type; for example, you need to know if the typedef is
>the structure itself, as shown above, or a pointer to the structure. In
>addition, they must be declared exactly once, whereas an
>incomplete structure type can be mentioned as many times as necessary.
>Typedefs are difficult to use in stand-alone header files.
The use of typedefs is not problematic IMO as they do provide a bit
shorter text, but they allow defining a clean method to define new types.
The problem I see in the description here is we do not use the _t to
denote a typedef type and we should when defining a typedef. If we define
the following:
typedef struct my_struct_s {
/* Š */
} my_struct_t;
Do not do the following:
struct my_struct {
/* Š */
};
typedef struct my_struct my_struct_t;
I like to use the _s for the structure tag name to denote it is the
non-typedef name and use the _t to denote the typedef. Not having the _s
is OK, but I would prefer the _s format. Splitting up the struct define
and typedef is not a problem, but some like to define all of the
structures at the top and then list all of the typedefs later. I prefer
them together to make the code more readable.
>The header that defines the typedef must be included before the header
>that uses it, or by the header that uses it (which causes namespace
>pollution),
Namespace pollution, I do not fully understand the concern here.
>
>or there must be a back-door mechanism for obtaining the typedef.
As for the ordering problem of typedefs and when they get included we
should attempt to order the headers correctly, but in some odd cases the
use of the Œstruct my_struct_s¹ instead of the typedef is OK, but we
should try to fix the ordering problem. Today we have Œstruct my_struct¹
to forward declare structures when the header is not included, which seems
to be a back-door solution, correct?
>
>NOTE #defines used instead of typedefs also are problematic (since they
>do not propagate the pointer type correctly due to direct text
>replacement).
>For example, ``#define pint int *`` does not work as expected, while
>``typedef int *pint`` does work. As stated when discussing macros,
>typedefs
>should be preferred to macros in cases like this.
>When convention requires a typedef; make its name match the struct tag.
>Avoid typedefs ending in ``_t``, except as specified in Standard C or by
>POSIX.
> /* Make the structure name match the typedef. */
> typedef struct bar {
> int level;
> } BAR;
> typedef int foo; /* This is foo. */
> typedef const long baz; /* This is baz. */
This one does not make sense the ŒBAR¹ to me looks like a MACRO not a
typedef, this is why using the Œbar_t' solution makes the most sense.
One more item having the pointer type in the typedef, which is a to me
hiding the pointer in the typedef and should be avoided at all cost.
typedef struct my_struct_s {
/* Š */
} *my_struct_t;
Or
typedef int * foobar;
Even when the typedef name tells you it is a pointer Œp_foobar¹ is not a
great solution as the p_ could mean something else and should be avoided.
When reading the code you would have to find the typedef or macro to
determine its type is just too difficult to work with IMO.
The above is just asking for trouble and confusion IMO as the developer
needs to make sure the typedef or macro is not hiding a pointer. We should
avoid at all cost hiding a pointer or [] in a typedef or macro.
Most of the functions in DPDK are like this:
int
rte_some_function(struct my_struct * foo, struct my_struct2 * bar, struct
my_struct3 * foorbar)
I believe the line is better in this fashion, less typing :-), but really
it helps in reading, plus the line may not have to be broken across two or
more lines.
int
rte_some_function(my_struct_t * foo, my_struct2_t * bar, my_struct3_t *
foobar)
(Not the best example, but you get the picture I hope)
BTW, I find having a space between the pointer and variable to be more
readable. The pointer is an attribute of the variable nothing more and not
having a space does not add to readability. Now having more then one space
between is way too much and can be very hard to read IMO.
int * foo;
char ** bar;
Should be:
int * foo;
char * bar;
Removing the space just makes the pointer symbol and variable name harder
to read IMO.
int *foo;
char *bar;
Some limit use of white space is good.
>
>
>C Function Definition, Declaration and Use
>
>Prototypes
>
>It is recommended, but not required that all functions are prototyped
>somewhere.
>
>Any function prototypes for private functions (that is, functions not
>used elsewhere) go at the top of the first source module. Functions
>local to one source module should be declared static.
>
>Functions used from other parts of code (external API) must be prototyped
>in the relevant include file.
>Function prototypes should be listed in a logical order, preferably
>alphabetical unless there is a compelling reason to use a different
>ordering.
>
>Functions that are used locally in more than one module go into a
>separate header file, for example, "extern.h".
>
>Do not use the ``__P`` macro.
>
>Functions that are part of an external API should be documented using
>Doxygen-like comments above declarations. See the Doxgen documentation
>topic for details.
>
>Associate names with parameter types, for example:
> void function(int fd);
>
>
>Short function prototypes should be contained on a single line. Longer
>prototypes, e.g. those with many parameters,
>can be split across multiple lines. Multi-line prototypes should use
>space-indentation to enable function parameters to line up:
> static char *function1(int _arg, const char *_arg2,
> struct foo *_arg3,
> struct bar *_arg4,
> struct baz *_arg5);
> static void usage(void);
>
>
>Definitions
>-----------
>
>The function type should be on a line by itself preceding the function.
>The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
>
>
>Do not declare functions inside other functions. ANSI C states that such
>declarations have file scope regardless of the nesting of the
>declaration.
>Hiding file declarations in what appears to be a local scope is
>undesirable and will elicit complaints from a good compiler.
>
>Old-style (K&R) function declaration should not be used, use ANSI
>function declarations instead as shown below. Long argument lists
>should be wrapped as described above in the function prototypes section.
> /*
> * All major routines should have a comment briefly describing what
> * they do. The comment before the "main" routine should describe
> * what the program does.
> */
> int
> main(int argc, char *argv[])
> {
> char *ep;
> long num;
> int ch;
>
>
>C Command Line Parsing
>----------------------
>
>For consistency, getopt(3) should be used to parse options. Options
>should be sorted in the getopt(3) call and the switch statement,
>unless parts of the switch cascade. Elements in a switch statement that
>cascade should have a FALLTHROUGH comment.
>Numerical arguments should be checked for accuracy. Code that cannot be
>reached should have a NOTREACHED comment.
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
> switch (ch) { /* Indent the switch. */
> case 'a': /* Don't indent the case. */
> aflag = 1; /* Indent case body one tab. */
> /* FALLTHROUGH */
> case 'b':
> bflag = 1;
> break;
> case 'N':
> Nflag = 1;
> break;
> case 'n':
> num = strtol(optarg, &ep, 10);
> if (num <= 0 || *ep != '\0') {
> warnx("illegal number, -n argument -- %s",
> optarg);
> usage();
> }
> break;
> case '?':
> default:
> usage();
> /* NOTREACHED */
> }
> argc -= optind;
> argv += optind;
>
>
>
>
>
>C Indentation
>-------------
>
>Control Statements and Loops
>
>Include a space after keywords (if, while, for, return, switch). Do not
>use braces (``{`` and ``}``) for control statements with zero or just a
>single statement, unless that statement is more than a single line in
>which case the braces are permitted. Forever loops are done with for
>statements, not while statements.
> for (p = buf; *p != '\0'; ++p)
> ; /* nothing */
> for (;;)
> stmt;
> for (;;) {
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
> }
> for (;;) {
> if (cond)
> stmt;
> }
> if (val != NULL)
> val = realloc(val, newsize);
>
>
>Parts of a for loop may be left empty. It is recommended that you do not
>put declarations inside blocks unless the routine is unusually
>complicated.
> for (; cnt < 15; cnt++) {
> stmt1;
> stmt2;
> }
>
>
>Indentation is a hard tab, that is, a tab character, not a sequence of
>spaces.
>NOTE General rule in DPDK, use tabs for indentation, spaces for
>alignment.
>If you have to wrap a long statement, put the operator at the end of the
>line, and indent again. For control statements (if, while, etc.),
>it is recommended that the next line be indented by two tabs, rather than
>one, to prevent confusion as to whether the second line of the
>control statement forms part of the statement body or not. For
>non-control statements, this issue does not apply, so they can be
>indented
>by a single tab. However, a two-tab indent is recommended in this case
>also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
>Do not add whitespace at the end of a line.
>
>Closing and opening braces go on the same line as the else keyword.
>Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
>Function Calls
>--------------
>
>Do not use spaces after function names. Commas should have a space after
>them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)``
>characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
I find the following OK as it tends to be more readable:
if ( error != 0 )
exit( error );
Again some space can be more readable. We have a space key use it wisely
:-)
>
>
>Operators
>---------
>
>Unary operators do not require spaces, binary operators do. Do not use
>parentheses unless they are required for precedence or unless the
>statement is confusing without them. Remember that other people may be
>more easily confused than you.
>
>Exit
>
>Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
> }
>
>
>Local Variables
>---------------
>
>When declaring variables in functions, declare them sorted by size, then
>in alphabetical order. Multiple variables per line are OK.
>If a line overflows reuse the type keyword.
>
>Be careful to not obfuscate the code by initializing variables in the
>declarations, only the last variable on a line should be initialized.
>If multiple variables are to be initialised when defined, put one per
>line. Do not use function calls in initializers.
> int i = 0, j = 0, k = 0; /* bad, too many initializer */
>
> char a = 0; /* OK, one variable per line with initializer */
> char b = 0;
>
> float x, y = 0.0; /* OK, only last variable has initializer */
>
>
>Casts and sizeof
>
>Casts and sizeof statements are not followed by a space. Always write
>sizeof statements with parenthesis.
>The redundant parenthesis rules do not apply to sizeof(var) instances.
>
>C Style and Conventions
>
>NULL Pointers
>
>NULL is the preferred null pointer constant. Use NULL instead of ``(type
>*)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
>for example, in assignments. Use ``(type *)NULL`` in other contexts, in
>particular for all function args.
>(Casting is essential for variadic args and is necessary for other args
>if the function prototype might not be in scope.) Test pointers against
>NULL, for example, use::
> (p = f()) == NULL
>
>
>not::
> !(p = f())
>
>
>Do not use ! for tests unless it is a boolean, for example, use::
> if (*p == '\0')
>
>
>not::
> if (!*p)
>
>
>Return Value
>------------
>
>If possible, functions should return 0 on success and a negative value on
>error. The negative value should be ``-errno`` if relevant, for example,
>``-EINVAL``.
>
>Routines returning ``void *`` should not have their return values cast to
>any pointer type.
>(Typecasting can prevent the compiler from warning about missing
>prototypes as any implicit definition of a function returns int - which,
>unlike "void *" needs a typecast to assign to a pointer variable.)
>NOTE The above rule applies to malloc, as well as to DPDK functions.
>Values in return statements should be enclosed in parentheses.
>
>Logging and Errors
>------------------
>
>In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
>
>In a userland program that is not a DPDK application, use err(3) or
>warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
> }
>
>
>Variable Arguments List
>-----------------------
>
>Variable numbers of arguments should look like this:
> #include <stdarg.h>
>
> void
> vaf(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> STUFF;
> va_end(ap);
> /* No return needed for void functions. */
> }
>
> static void
> usage()
> {
> /* Insert an empty line if the function has no local variables.
>*/
>
>
>Printf
>------
>
>Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is
>faster and usually cleaner, and helps to avoid unnecessary bugs. However,
>be aware of format string bugs::
> int
> main(int argc, char **argv)
> {
> if(argc != 2)
> exit(1);
> printf(argv[1]); /* bad ! */
> printf("%s", argv[1]); /* correct */
>
>
>Usage
>-----
>
>Usage statements should look like the manual pages SYNOPSIS. The usage
>statement should be structured in the following order:
>1. Options without operands come first, in alphabetical order, inside a
>single set of brackets (``[`` and ``]``).
>2. Options with operands come next, also in alphabetical order, with each
>option and its argument inside its own pair of brackets.
>3. Required arguments (if any) are next, listed in the order they should
>be specified on the command line.
>4. Finally, any optional arguments, listed in the order they should be
>specified, and all inside brackets.
>
>A bar (`|') separates ``either-or`` options/arguments, and multiple
>options/arguments, which are specified together, are placed in a single
>set of brackets.
> "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
>
> (void)fprintf(stderr, "usage: f [-ab]\n");
> exit(1);
> }
>
>
>Note that the manual page options description should list the options in
>pure alphabetical order. That is, without regard to
>whether an option takes arguments or not. The alphabetical ordering
>should take into account the case ordering shown above.
>
>Branch Prediction
>-----------------
>
>When a test is done in a critical zone (called often or in a data path)
>use the ``likely()`` and ``unlikely()`` macros. They are expanded
>as a compiler builtin and allow the developer to indicate if the branch
>is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
>
>
>Static Variables and Functions
>------------------------------
>
>All functions and variables that are local to a file must be declared as
>``static`` because it can often help the compiler to do
>some optimizations (such as, inlining the code).
>
>Functions that must be inlined have to be declared as ``static inline``
>and can be defined in a .c or a .h file.
>
>Const Attribute
>---------------
>
>Particular care must be taken with the use of the ``const`` attribute. It
>should be used as often as possible when a variable is read-only.
>
>ASM Coding Rules
>----------------
>
>Assembly Syntax
>
>NASM is used for assembly, with the syntax, therefore guidelines given
>here are appropriate to this target.
>[GNU as is intended to support both syntax variants, but that is not
>documented here]. The following general guidelines are valid in any case.
> globals, extern and macros are to be defined at the top of the file
> labels should stay explicit, and are left aligned
> code is indented with a tabulation, no spaces
> instruction and operands should be separated by a tab too
> code should be separated in blocks
> blocks, when possible, should start with a comment explanation
>
>Sample code:
> ; comment header
>
> ; export this symbol
> [GLOBAL entry]
>
> ; external variables and functions
> [EXTERN variable]
>
> ; 16 bits code
> [BITS 16]
>
> ; macros like
> BIOS_START EQU 0x7C00
>
> entry:
>
> ; Clear interrupt flag
> cli
>
> ; Set segment registers to 0
> xor bx, bx
> mov es, bx
> mov fs, bx
> mov gs, bx
> mov ds, bx
> mov ss, bx
> mov sp, 0x7C00
> sti
>
>;; [...] snip [...]
>
>
>Use of C-style macros is allowed. When compiling ASM code, a file is
>parsed by the C preprocessor. It is then allowed to share some constants
>between C and assembly code, in a ``.h`` file.
>
>Inline ASM in C code
>
>The ``asm`` and ``volatile`` keywords do not have underscores. The AT&T
>syntax should be used. Input and output operands should be named to avoid
>confusion,
>as shown in the following example::
> asm volatile("outb %[val], %[port]"
> : :
> [port] "dN" (port),
> [val] "a" (val));
>
>
>Environment or Architecture-specific Sources
>
>In DPDK and DPDK applications, some code is specific to an architecture
>(i686, x86_64) or to an executive environment (bare-metal or linuxapp)
>and so on.
>
>There are several ways to handle specific code:
> Use a ``#ifdef`` with the CONFIG option in the C code. This can be done
>when the differences are small and they can be embedded in the same C
>file::
> #ifdef RTE_ARCH_I686
> toto();
> #else
> titi();
> #endif
>
>Use the CONFIG option in the Makefile. This is done when the differences
>are more significant. In this case, the code is split into
>two separate files that are architecture or environment specific.
>
>The same logic applies to header files.
>
>By convention, a file is common if it is not located in a directory
>indicating that it is specific. For instance, a file located in a
>subdir of "x86_64" directory is specific to this architecture. A file
>located in a subdir of "linuxapp" is specific to this execution
>environment.
>NOTE As in the linux kernel, the "CONFIG_" prefix is not used in C code.
>This is only needed in Makefiles or shell scripts.
>Per Architecture Sources
>
>The following config options can be used:
> CONFIG_RTE_ARCH is a string that contains the name of the architecture.
> CONFIG_RTE_ARCH_I686, CONFIG_RTE_ARCH_X86_64 or
>CONFIG_RTE_ARCH_X86_64_32 are defined only if we are building for those
>architectures.
>
>Per Execution Environment Sources
>
>The following config options can be used:
> CONFIG_RTE_EXEC_ENV is a string that contains the name of the executive
>environment.
> CONFIG_RTE_EXEC_ENV_BAREMETAL or CONFIG_RTE_EXEC_ENV_LINUXAPP are
>defined only if we are building for this execution environment.
>
>Per Driver Sources
> RTE_LIBRTE_IGB_PMD or RTE_LIBRTE_IXGBE_PMD are defined only if we are
>using this driver.
>
>Doxygen Documentation
>---------------------
>
>The API documentation is automatically generated in the DPDK framework.
>That is why all files that are part of the public
>API must be documented using Doxygen syntax.
>
>The public API comprises functions of DPDK that can be used by an
>external application that will use the SDK. Only the Doxygen
>syntax described in the coding rules (this document) should be used in
>the code. All the Doxygen features are described in the Doxygen manual
>online.
>
>Documenting a Function
>
>All public functions must be documented. The documentation is placed in
>the header file, above the declaration of the function.
>The definition of the function may be documented, but using standard
>comments (not in doxygen format).
>Private functions can be documented using Doxygen. The following is an
>example of function documentation:
> /**
> * Summary here; one sentence on one line (should not exceed 80 chars).
> *
> * A more detailed description goes here.
> *
> * A blank line forms a paragraph. There should be no trailing
>white-space
> * anywhere.
> *
> * @param first
> * "@param" is a Doxygen directive to describe a function parameter.
>Like
> * some other directives, it takes a term/summary on the same line and
>a
> * description (this text) indented by 2 spaces on the next line. All
> * descriptive text should wrap at 80 chars, without going over.
> * Newlines are NOT supported within directives; if a newline would be
> * before this text, it would be appended to the general description
>above.
> * @param second
> * There should be no newline between multiple directives (of the same
> * type).
> *
> * @return
> * "@return" is a different Doxygen directive to describe the return
>value
> * of a function, if there is any.
> */
> int rte_foo(int first, int second)
>
>
>Documenting Files
>
>Each public file may start with a comment describing what the file does.
>For example:
> /**
> * @file
> * This file describes the coding rules of RTE.
> *
> * It contains the coding rules of C code, ASM code, reStructured
> * Text documentation, and of course how to use doxygen to document
> * public API.
> */
>
>
>Documenting Constants and Variables
> /**
> * The definition of a funny TRUE.
> */
> #define TRUE 0
>
>
>
>
> #define TRUE 1 /**< another way to document a macro */
>
> /**
> * Frequency of the HPET counter in Hz
> *
> * @see rte_eal_hpet_init()
> */
> extern uint64_t eal_hpet_resolution_hz;
>
>
>Documenting Structures
>
>Public structures should also be documented. The ``/**<`` sequence can be
>used to documented the fields of the structure, as shown in the following
>example:
> /**
> * Structure describing a memzone, which is a contiguous portions of
> * physical memory identified by a name.
> */
> struct rte_memzone {
>
> #define MEMZONE_NAMESIZE 32
> char name[MEMZONE_NAMESIZE]; /**< name of the memory zone */
>
> phys_addr_t phys_addr; /**< start physical address */
> void *addr; /**< start virtual address */
> uint64_t len; /**< len of the memzone */
>
> int socket_id; /**< NUMA socket id */
> };
>
>
>Using Lists
>
>Using the minus character, it is possible to generate a bullet list. The
>minus signs must be column-aligned. If the minus sign is followed by a
>hash,
>then it generates an enumerated list. Refer to the official Doxygen
>documentation for more information.
> /**
> * A list of events:
> * - mouse events
> * -# mouse move event
> * -# mouse click event\n
> * More info about the click event.
> * -# mouse double click event
> * - keyboard events
> * -# key down event
> * -# key up event
> *
> * More text here.
> */
>
>
>See Also Sections
>
>The @see keyword can be used to highlight a link to an existing function,
>file, or URL. This directive should be placed on one line, without
>anything else, at the bottom of the documentation header.
> /**
> * (documentation of function, file, ...)
> *
> * @see rte_foo()
> * @see eal_memzone.c
> */
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 14:16 ` Wiles, Keith
@ 2015-04-14 14:50 ` Bruce Richardson
0 siblings, 0 replies; 62+ messages in thread
From: Bruce Richardson @ 2015-04-14 14:50 UTC (permalink / raw)
To: Wiles, Keith; +Cc: dev
On Wed, Apr 08, 2015 at 02:16:55PM +0000, Wiles, Keith wrote:
>
>
> On 4/8/15, 5:43 AM, "Butler, Siobhan A" <siobhan.a.butler@intel.com> wrote:
>
> >Hi all,
> >To add to the tools brainstorming - I propose we use the following Coding
> >Standards as the basis of guidelines on coding style going forward.
> >The style outlined below is in alignment with the current convention used
> >for the majority of the project.
> >Any thoughts/suggestions or feedback welcome.
> >Thanks
> >Siobhan :)
> ><siobhan.a.butler@intel.com>
> >
> >
> >
> >Coding Style
> >~~~~~~~~~~
> >
> >Description
> >-----------
> >
> >This document specifies the preferred style for source files in the DPDK
> >source tree.
> >It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
> >Kernel Developer's Manual (see man style(9)),
> >but was heavily modified for the needs of the DPDK. Many of the style
> >rules are implicit in the examples.
> >Be careful to check the examples before assuming that style is silent on
> >an issue.
> >
> >General Guidelines
> >------------------
> >
> >The rules and guidelines given in this document cannot cover every
> >situation, so the following general guidelines should be used as a
> >fallback:
> >The code style should be consistent within each individual file, and
> >within each file in a given directory or module - in the case of creating
> >new files
> >The primary reason for coding standards is to increase code readability
> >and comprehensibility, therefore always use whatever option will make the
> >code easiest to read.
> >
> >The following more specific recommendations apply to all sections, both
> >for C and assembly code:
> >Line length is recommended to be not more than 80 characters, including
> >comments. [Tab stop size should be assumed to be at least 4-characters
> >wide]
> >Indentation should be to no more than 3 levels deep.
> >NOTE The above are recommendations, and not hard limits. However, it is
> >expected that the recommendations should be followed in all but the
> >rarest situations.
> >C Comment Style
> >
> >Usual Comments
> >--------------
> >
> >These comments should be used in normal cases. To document a public API,
> >a doxygen-like format must be used: refer to Doxygen Documentation.
> > /*
> > * VERY important single-line comments look like this.
> > */
> >
> > /* Most single-line comments look like this. */
> >
> > /*
> > * Multi-line comments look like this. Make them real sentences. Fill
> > * them so they look like real paragraphs.
> > */
>
> Did you mean to have the Œ*¹ aligned, if so good, if not then it does not
> make sense to not align them. The indent of one space here does not help
> convey any information IMO.
> >
> >License Header
> >--------------
> >
> >Each file should begin with a special comment tag which will contain the
> >appropriate copyright and license for the file (Generally BSD License).
> >After any copyright header, a blank line should be left before any other
> >contents, e.g. include statements in a C file.
> >
> >C Preprocessor Directives
> >-------------------------
> >
> >Header Includes
> >
> >In DPDK sources, the include files should be ordered as following:
> > libc includes (system includes first)
> > DPDK EAL includes
> > DPDK misc libraries includes
> > application-specific includes
> >
> >Example:
> > #include <stdio.h>
> > #include <stdlib.h>
> >
> > #include <rte_eal.h>
> >
> > #include <rte_ring.h>
> > #include <rte_mempool.h>
> >
> > #include "application.h"
> >
> >
> >Global pathnames are defined in <paths.h>. Pathnames local to the program
> >go in "pathnames.h" in the local directory.
> > #include <paths.h>
> >
> >
> >Leave another blank line before the user include files.
> > #include "pathnames.h" /* Local includes in double quotes. */
> >
> >NOTE Please avoid, as much as possible, including headers from other
> >headers file. Doing so should be properly explained and justified.
> >Headers should be protected against multiple inclusion with the usual:
> > #ifndef _FILE_H_
> > #define _FILE_H_
> >
> > /* Code */
> >
> > #endif /* _FILE_H_ */
> >
> >
> >Macros
> >
> >Do not ``#define`` or declare names in the implementation namespace
> >except for implementing application interfaces.
> >
> >The names of ``unsafe`` macros (ones that have side effects), and the
> >names of macros for manifest constants, are all in uppercase.
> >
> >The expansions of expression-like macros are either a single token or
> >have outer parentheses. If a macro is an inline expansion of a function,
> >the function name is all in lowercase and the macro has the same name all
> >in uppercase. Right-justify the backslashes;
> >it makes it easier to read. If the macro encapsulates a compound
> >statement, enclose it in a do loop, so that it can be used safely in if
> >statements.
> >Any final statement-terminating semicolon should be supplied by the macro
> >invocation rather than the macro, to make parsing easier for
> >pretty-printers and editors.
> > #define MACRO(x, y) do { \
> > variable = (x) + (y); \
> > (y) += 2; \
> > }while (0)
>
> I have seen this Œwhile( /*CONSTCOND*/0 )¹ (I think that is the comment)
> plus I have see this Œwhile((0))¹ as gcc complained about a constant in
> the while(). This one maybe fixed at this point in the compilers.
I've never seen a compiler error complaining about a do { } while (0), since this
is the generally recommended way to do define code blocks in macros.
> >
> >NOTE Wherever possible, enums and typedefs should be preferred to macros,
> >since they provide additional degrees
> >of type-safety and can allow compilers to emit extra warnings about
> >unsafe code.
> >
> >Conditional Compilation
> >-----------------------
> >
> >When code is conditionally compiled using #ifdef or #if, a comment may be
> >added following the matching #endif or #else to
> >permit the reader to easily discern where conditionally compiled code
> >regions end. This comment should be used only for
> >(subjectively) long regions, regions greater than 20 lines, or where a
> >series of nested #ifdef 's may be confusing to the reader.
> >Exceptions may be made for cases where code is conditionally not compiled
> >for the purposes of lint(1), even though the uncompiled
> >region may be small. The comment should be separated from the #endif or
> >#else by a single space. For short conditionally compiled regions,
> >a closing comment should not be used.
> >
> >The comment for #endif should match the expression used in the
> >corresponding #if or #ifdef. The comment for #else and #elif
> >should match the inverse of the expression(s) used in the preceding #if
> >and/or #elif statements. In the comments,
> >the subexpression defined(FOO) is abbreviated as FOO. For the purposes of
> >comments, #ifndef FOO is treated as #if !defined(FOO).
> > #ifdef KTRACE
> > #include <sys/ktrace.h>
> > #endif
> >
> > #ifdef COMPAT_43
> > /* A large region here, or other conditional code. */
> > #else /* !COMPAT_43 */
> > /* Or here. */
> > #endif /* COMPAT_43 */
> >
> > #ifndef COMPAT_43
> > /* Yet another large region here, or other conditional code. */
> > #else /* COMPAT_43 */
> > /* Or here. */
> > #endif /* !COMPAT_43 */
> >
> >NOTE Conditional compilation should be used only when absolutely
> >necessary, as it increases the number of target binaries that need to be
> >built and tested.
> >C Types
> >
> >Integers
> >
> >For fixed/minimum-size integer values, the project uses the form uintXX_t
> >(from stdint.h) instead of older BSD-style integer identifiers of the
> >form u_intXX_t.
> >
> >Enumerations
> >------------
> >
> >Enumeration values are all uppercase.
> > enum enumtype { ONE, TWO } et;
> >
> >
> >Bitfields
> >---------
> >
> >The developer should group bitfields that are included in the same
> >integer, as follows:
> > struct grehdr {
> > uint16_t rec:3,
> > srr:1,
> > seq:1,
> > key:1,
> > routing:1,
> > csum:1,
> > version:3,
> > reserved:4,
> > ack:1;
> > /* ... */
> > }
>
> I like to line up the Œ:¹ just to make it a bit more readable, but I can
> live without it.
> >
> >
> >Variable Declarations
> >---------------------
> >
> >In declarations, do not put any whitespace between asterisks and adjacent
> >tokens, except for tokens that are identifiers related to types.
>
> Much more readable to have the space between pointer and token IMO.
>
This is not the way things are done right now in our code, and I see no reason
to change it. [Does checkpatch complain if you insert spaces?]
> >
> >(These identifiers are the names of basic types, type qualifiers, and
> >typedef-names other than the one being declared.)
> >Separate these identifiers from asterisks using a single space.
>
> Not sure I understand here, can you give an example?
const int *x; /* no space between asterisk and variable name token */
const int * const x; /* space between asterisk and keyword */
>
> >
> >
> >Structure Declarations
> >
> >When declaring variables in structures, declare them sorted by use, then
> >by size (largest to smallest), and then in alphabetical order.
> >Alignment constraints may override the previous rules. The first category
> >normally does not apply, but there are exceptions.
> >Each structure element gets its own line. Try to make the structure
> >readable by aligning the member names using spaces as shown below.
> >Names following extremely long types, which therefore cannot be easily
> >aligned with the rest, should be separated by a single space.
>
> Sorting variables in structures and other places just seem to much
> trouble. Sometimes you want to group variables together as they are used
> together in the code below.
Hence the "sorted by use" - where appropriate.
>
> Ordering structure members is not going work as alignment and grouping is
> more important. Alignment is important as we all know holes in structures
> which can cause some very odd bugs from arch to arch. Also order of the
> members to a structure could be dependent on hardware or some other
> overlay structure being used to reference that structure. The comments
> should point this issue out in case someone decides to change the order.
> My main concern is trying to keep up with fact a set of members in one
> structure are sorted and in another they are not for some reason.
Not sure I understand what the issue is here. Where "by use" does not apply,
i.e. in most cases, we hope, we group by size from largest to smallest, thereby
avoiding wasting space with packing i.e. holes. Overall, I think the above
guideline is pretty reasonable.
> Sorting local variables is also not reasonable, for grouping of variables
> and being able to maintain the order.
>
> >
> > struct foo {
> > struct foo *next; /* List of active foo. */
> > struct mumble amumble; /* Comment for mumble. */
> > int bar; /* Try to align the comments. */
> > struct verylongtypename *baz; /* Won't fit with other members
> >*/
> > };
> >
> >
> >Major structures should be declared at the top of the file in which they
> >are used, or in separate header files if they are used
> >in multiple source files. Use of the structures should be by separate
> >declarations and should be extern if they are declared in a header file.
> >
> >Queues
> >
> >Use queue(3) macros rather than rolling your own lists, whenever
> >possible. Thus, the previous example would be better written:
> > #include <sys/queue.h>
> >
> > struct foo {
> > LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> > struct mumble amumble; /* Comment for mumble. */
> > int bar; /* Try to align the comments. */
> > struct verylongtypename *baz; /* Won't fit with other members
> >*/
> > };
> > LIST_HEAD(, foo) foohead; /* Head of global foo list. */
> >
> >
> >DPDK also provides an optimized way to store elements in lockless rings.
> >This should be used in all data-path code, when there are several
> >consumer and/or producers to avoid locking for concurrent access.
> >
> >Typedefs
> >
> >Avoid using typedefs for structure types. For example, use:
> > struct my_struct_type {
> > /* ... */
> > };
> >
> > struct my_struct_type my_var;
> >
> >
> >rather than:
> > typedef struct my_struct_type {
> > /* ... */
> > } my_struct_type;
> >
> > my_struct_type my_var
> >
> >
> >Typedefs are problematic because they do not properly hide their
> >underlying type; for example, you need to know if the typedef is
> >the structure itself, as shown above, or a pointer to the structure. In
> >addition, they must be declared exactly once, whereas an
> >incomplete structure type can be mentioned as many times as necessary.
> >Typedefs are difficult to use in stand-alone header files.
>
> The use of typedefs is not problematic IMO as they do provide a bit
> shorter text, but they allow defining a clean method to define new types.
> The problem I see in the description here is we do not use the _t to
> denote a typedef type and we should when defining a typedef. If we define
> the following:
>
> typedef struct my_struct_s {
> /* Š */
> } my_struct_t;
>
> Do not do the following:
>
> struct my_struct {
> /* Š */
> };
> typedef struct my_struct my_struct_t;
>
> I like to use the _s for the structure tag name to denote it is the
> non-typedef name and use the _t to denote the typedef. Not having the _s
> is OK, but I would prefer the _s format. Splitting up the struct define
> and typedef is not a problem, but some like to define all of the
> structures at the top and then list all of the typedefs later. I prefer
> them together to make the code more readable.
>
DPDK does not currently use typedefs, except for function pointer types, which are
more awkward than normal to deal with. I see no real reason to start using them,
as I believe the objections to their use given in the proposed document are
still valid.
I also think having to resort to prefixes is ugly, and if we did start using
typedefs with _t prefixes in place of structures, we end up with very different
looking code meaning either:
* we have a codebase with two very different look and feels, or
* we end up with major rework to redo all code to the new style.
> >The header that defines the typedef must be included before the header
> >that uses it, or by the header that uses it (which causes namespace
> >pollution),
>
> Namespace pollution, I do not fully understand the concern here.
You should not have to include a whole header file just to get a particular
structure definition that you use just a pointer to. Doing so, just fills up
your namespace with all the other definitions in the header file you don't need.
Better instead to just put in a single line saying e.g.:
struct rte_mbuf;
> >
> >or there must be a back-door mechanism for obtaining the typedef.
>
> As for the ordering problem of typedefs and when they get included we
> should attempt to order the headers correctly, but in some odd cases the
> use of the Œstruct my_struct_s¹ instead of the typedef is OK, but we
> should try to fix the ordering problem. Today we have Œstruct my_struct¹
> to forward declare structures when the header is not included, which seems
> to be a back-door solution, correct?
>
Or we can just continue to not use typedefs, in which case there is no issue :-)
>
> >
> >NOTE #defines used instead of typedefs also are problematic (since they
> >do not propagate the pointer type correctly due to direct text
> >replacement).
> >For example, ``#define pint int *`` does not work as expected, while
> >``typedef int *pint`` does work. As stated when discussing macros,
> >typedefs
> >should be preferred to macros in cases like this.
> >When convention requires a typedef; make its name match the struct tag.
> >Avoid typedefs ending in ``_t``, except as specified in Standard C or by
> >POSIX.
> > /* Make the structure name match the typedef. */
> > typedef struct bar {
> > int level;
> > } BAR;
> > typedef int foo; /* This is foo. */
> > typedef const long baz; /* This is baz. */
>
> This one does not make sense the ŒBAR¹ to me looks like a MACRO not a
> typedef, this is why using the Œbar_t' solution makes the most sense.
>
> One more item having the pointer type in the typedef, which is a to me
> hiding the pointer in the typedef and should be avoided at all cost.
>
> typedef struct my_struct_s {
> /* Š */
> } *my_struct_t;
>
> Or
>
> typedef int * foobar;
>
> Even when the typedef name tells you it is a pointer Œp_foobar¹ is not a
> great solution as the p_ could mean something else and should be avoided.
> When reading the code you would have to find the typedef or macro to
> determine its type is just too difficult to work with IMO.
>
> The above is just asking for trouble and confusion IMO as the developer
> needs to make sure the typedef or macro is not hiding a pointer. We should
> avoid at all cost hiding a pointer or [] in a typedef or macro.
>
This is exactly what the document is stating - except doing so via just not
using typedefs or macros for types. :-)
> Most of the functions in DPDK are like this:
>
> int
> rte_some_function(struct my_struct * foo, struct my_struct2 * bar, struct
> my_struct3 * foorbar)
>
> I believe the line is better in this fashion, less typing :-), but really
> it helps in reading, plus the line may not have to be broken across two or
> more lines.
>
> int
> rte_some_function(my_struct_t * foo, my_struct2_t * bar, my_struct3_t *
> foobar)
>
> (Not the best example, but you get the picture I hope)
>
> BTW, I find having a space between the pointer and variable to be more
> readable. The pointer is an attribute of the variable nothing more and not
> having a space does not add to readability. Now having more then one space
> between is way too much and can be very hard to read IMO.
>
> int * foo;
> char ** bar;
>
> Should be:
>
> int * foo;
> char * bar;
>
> Removing the space just makes the pointer symbol and variable name harder
> to read IMO.
>
> int *foo;
> char *bar;
>
> Some limit use of white space is good.
>
> >
> >
> >C Function Definition, Declaration and Use
> >
> >Prototypes
> >
> >It is recommended, but not required that all functions are prototyped
> >somewhere.
> >
> >Any function prototypes for private functions (that is, functions not
> >used elsewhere) go at the top of the first source module. Functions
> >local to one source module should be declared static.
> >
> >Functions used from other parts of code (external API) must be prototyped
> >in the relevant include file.
> >Function prototypes should be listed in a logical order, preferably
> >alphabetical unless there is a compelling reason to use a different
> >ordering.
> >
> >Functions that are used locally in more than one module go into a
> >separate header file, for example, "extern.h".
> >
> >Do not use the ``__P`` macro.
> >
> >Functions that are part of an external API should be documented using
> >Doxygen-like comments above declarations. See the Doxgen documentation
> >topic for details.
> >
> >Associate names with parameter types, for example:
> > void function(int fd);
> >
> >
> >Short function prototypes should be contained on a single line. Longer
> >prototypes, e.g. those with many parameters,
> >can be split across multiple lines. Multi-line prototypes should use
> >space-indentation to enable function parameters to line up:
> > static char *function1(int _arg, const char *_arg2,
> > struct foo *_arg3,
> > struct bar *_arg4,
> > struct baz *_arg5);
> > static void usage(void);
> >
> >
> >Definitions
> >-----------
> >
> >The function type should be on a line by itself preceding the function.
> >The opening brace of the function body should be on a line by itself.
> > static char *
> > function(int a1, int a2, float fl, int a4)
> > {
> >
> >
> >Do not declare functions inside other functions. ANSI C states that such
> >declarations have file scope regardless of the nesting of the
> >declaration.
> >Hiding file declarations in what appears to be a local scope is
> >undesirable and will elicit complaints from a good compiler.
> >
> >Old-style (K&R) function declaration should not be used, use ANSI
> >function declarations instead as shown below. Long argument lists
> >should be wrapped as described above in the function prototypes section.
> > /*
> > * All major routines should have a comment briefly describing what
> > * they do. The comment before the "main" routine should describe
> > * what the program does.
> > */
> > int
> > main(int argc, char *argv[])
> > {
> > char *ep;
> > long num;
> > int ch;
> >
> >
> >C Command Line Parsing
> >----------------------
> >
> >For consistency, getopt(3) should be used to parse options. Options
> >should be sorted in the getopt(3) call and the switch statement,
> >unless parts of the switch cascade. Elements in a switch statement that
> >cascade should have a FALLTHROUGH comment.
> >Numerical arguments should be checked for accuracy. Code that cannot be
> >reached should have a NOTREACHED comment.
> > while ((ch = getopt(argc, argv, "abNn:")) != -1)
> > switch (ch) { /* Indent the switch. */
> > case 'a': /* Don't indent the case. */
> > aflag = 1; /* Indent case body one tab. */
> > /* FALLTHROUGH */
> > case 'b':
> > bflag = 1;
> > break;
> > case 'N':
> > Nflag = 1;
> > break;
> > case 'n':
> > num = strtol(optarg, &ep, 10);
> > if (num <= 0 || *ep != '\0') {
> > warnx("illegal number, -n argument -- %s",
> > optarg);
> > usage();
> > }
> > break;
> > case '?':
> > default:
> > usage();
> > /* NOTREACHED */
> > }
> > argc -= optind;
> > argv += optind;
> >
> >
> >
> >
> >
> >C Indentation
> >-------------
> >
> >Control Statements and Loops
> >
> >Include a space after keywords (if, while, for, return, switch). Do not
> >use braces (``{`` and ``}``) for control statements with zero or just a
> >single statement, unless that statement is more than a single line in
> >which case the braces are permitted. Forever loops are done with for
> >statements, not while statements.
> > for (p = buf; *p != '\0'; ++p)
> > ; /* nothing */
> > for (;;)
> > stmt;
> > for (;;) {
> > z = a + really + long + statement + that + needs +
> > two + lines + gets + indented + on + the +
> > second + and + subsequent + lines;
> > }
> > for (;;) {
> > if (cond)
> > stmt;
> > }
> > if (val != NULL)
> > val = realloc(val, newsize);
> >
> >
> >Parts of a for loop may be left empty. It is recommended that you do not
> >put declarations inside blocks unless the routine is unusually
> >complicated.
> > for (; cnt < 15; cnt++) {
> > stmt1;
> > stmt2;
> > }
> >
> >
> >Indentation is a hard tab, that is, a tab character, not a sequence of
> >spaces.
> >NOTE General rule in DPDK, use tabs for indentation, spaces for
> >alignment.
> >If you have to wrap a long statement, put the operator at the end of the
> >line, and indent again. For control statements (if, while, etc.),
> >it is recommended that the next line be indented by two tabs, rather than
> >one, to prevent confusion as to whether the second line of the
> >control statement forms part of the statement body or not. For
> >non-control statements, this issue does not apply, so they can be
> >indented
> >by a single tab. However, a two-tab indent is recommended in this case
> >also to keep consistency across all statement types.
> > while (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){
> > x = y + z; /* control stmt body lines up with second line of */
> > a = b + c; /* control statement itself if single indent used */
> > }
> >
> > if (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){ /* two tabs used */
> > x = y + z; /* statement body no longer lines up */
> > a = b + c;
> > }
> >
> > z = a + really + long + statement + that + needs +
> > two + lines + gets + indented + on + the +
> > second + and + subsequent + lines;
> >
> >
> >Do not add whitespace at the end of a line.
> >
> >Closing and opening braces go on the same line as the else keyword.
> >Braces that are not necessary should be left out.
> > if (test)
> > stmt;
> > else if (bar) {
> > stmt;
> > stmt;
> > } else
> > stmt;
> >
> >
> >Function Calls
> >--------------
> >
> >Do not use spaces after function names. Commas should have a space after
> >them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)``
> >characters.
> > error = function(a1, a2);
> > if (error != 0)
> > exit(error);
>
> I find the following OK as it tends to be more readable:
> if ( error != 0 )
> exit( error );
>
> Again some space can be more readable. We have a space key use it wisely
> :-)
While I would be ok with making a change like this if everyone likes it, I'm not
sure it helps that much. It's downsides are:
* we break compatibility with the majority of our existing codebase
* we would have to fork checkpatch to stop throwing errors at something like this.
The downsides seem to outweigh the positives for me.
> >
> >
> >Operators
> >---------
> >
> >Unary operators do not require spaces, binary operators do. Do not use
> >parentheses unless they are required for precedence or unless the
> >statement is confusing without them. Remember that other people may be
> >more easily confused than you.
> >
> >Exit
> >
> >Exits should be 0 on success, or 1 on failure.
> > exit(0); /*
> > * Avoid obvious comments such as
> > * "Exit 0 on success."
> > */
> > }
> >
> >
> >Local Variables
> >---------------
> >
> >When declaring variables in functions, declare them sorted by size, then
> >in alphabetical order. Multiple variables per line are OK.
> >If a line overflows reuse the type keyword.
> >
> >Be careful to not obfuscate the code by initializing variables in the
> >declarations, only the last variable on a line should be initialized.
> >If multiple variables are to be initialised when defined, put one per
> >line. Do not use function calls in initializers.
> > int i = 0, j = 0, k = 0; /* bad, too many initializer */
> >
> > char a = 0; /* OK, one variable per line with initializer */
> > char b = 0;
> >
> > float x, y = 0.0; /* OK, only last variable has initializer */
> >
> >
> >Casts and sizeof
> >
> >Casts and sizeof statements are not followed by a space. Always write
> >sizeof statements with parenthesis.
> >The redundant parenthesis rules do not apply to sizeof(var) instances.
> >
> >C Style and Conventions
> >
> >NULL Pointers
> >
> >NULL is the preferred null pointer constant. Use NULL instead of ``(type
> >*)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
> >for example, in assignments. Use ``(type *)NULL`` in other contexts, in
> >particular for all function args.
> >(Casting is essential for variadic args and is necessary for other args
> >if the function prototype might not be in scope.) Test pointers against
> >NULL, for example, use::
> > (p = f()) == NULL
> >
> >
> >not::
> > !(p = f())
> >
> >
> >Do not use ! for tests unless it is a boolean, for example, use::
> > if (*p == '\0')
> >
> >
> >not::
> > if (!*p)
> >
> >
> >Return Value
> >------------
> >
> >If possible, functions should return 0 on success and a negative value on
> >error. The negative value should be ``-errno`` if relevant, for example,
> >``-EINVAL``.
> >
> >Routines returning ``void *`` should not have their return values cast to
> >any pointer type.
> >(Typecasting can prevent the compiler from warning about missing
> >prototypes as any implicit definition of a function returns int - which,
> >unlike "void *" needs a typecast to assign to a pointer variable.)
> >NOTE The above rule applies to malloc, as well as to DPDK functions.
> >Values in return statements should be enclosed in parentheses.
> >
> >Logging and Errors
> >------------------
> >
> >In the DPDK environment, use the logging interface provided::
> > #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> > #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
> >
> > /* enable these logs type */
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
> >
> > /* log in debug level */
> > rte_set_log_level(RTE_LOG_DEBUG);
> > RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> > RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> > RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
> >
> > /* log in info level */
> > rte_set_log_level(RTE_LOG_INFO);
> > RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
> >
> >
> >In a userland program that is not a DPDK application, use err(3) or
> >warn(3). Do not create your own variant.
> > if ((four = malloc(sizeof(struct foo))) == NULL)
> > err(1, (char *)NULL);
> > if ((six = (int *)overflow()) == NULL)
> > errx(1, "number overflowed");
> > return (eight);
> > }
> >
> >
> >Variable Arguments List
> >-----------------------
> >
> >Variable numbers of arguments should look like this:
> > #include <stdarg.h>
> >
> > void
> > vaf(const char *fmt, ...)
> > {
> > va_list ap;
> >
> > va_start(ap, fmt);
> > STUFF;
> > va_end(ap);
> > /* No return needed for void functions. */
> > }
> >
> > static void
> > usage()
> > {
> > /* Insert an empty line if the function has no local variables.
> >*/
> >
> >
> >Printf
> >------
> >
> >Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is
> >faster and usually cleaner, and helps to avoid unnecessary bugs. However,
> >be aware of format string bugs::
> > int
> > main(int argc, char **argv)
> > {
> > if(argc != 2)
> > exit(1);
> > printf(argv[1]); /* bad ! */
> > printf("%s", argv[1]); /* correct */
> >
> >
> >Usage
> >-----
> >
> >Usage statements should look like the manual pages SYNOPSIS. The usage
> >statement should be structured in the following order:
> >1. Options without operands come first, in alphabetical order, inside a
> >single set of brackets (``[`` and ``]``).
> >2. Options with operands come next, also in alphabetical order, with each
> >option and its argument inside its own pair of brackets.
> >3. Required arguments (if any) are next, listed in the order they should
> >be specified on the command line.
> >4. Finally, any optional arguments, listed in the order they should be
> >specified, and all inside brackets.
> >
> >A bar (`|') separates ``either-or`` options/arguments, and multiple
> >options/arguments, which are specified together, are placed in a single
> >set of brackets.
> > "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> > "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
> >
> > (void)fprintf(stderr, "usage: f [-ab]\n");
> > exit(1);
> > }
> >
> >
> >Note that the manual page options description should list the options in
> >pure alphabetical order. That is, without regard to
> >whether an option takes arguments or not. The alphabetical ordering
> >should take into account the case ordering shown above.
> >
> >Branch Prediction
> >-----------------
> >
> >When a test is done in a critical zone (called often or in a data path)
> >use the ``likely()`` and ``unlikely()`` macros. They are expanded
> >as a compiler builtin and allow the developer to indicate if the branch
> >is likely to be taken or not. Example:
> > #include <rte_branch_prediction.h>
> > if (likely(x > 1))
> > do_stuff();
> >
> >
> >Static Variables and Functions
> >------------------------------
> >
> >All functions and variables that are local to a file must be declared as
> >``static`` because it can often help the compiler to do
> >some optimizations (such as, inlining the code).
> >
> >Functions that must be inlined have to be declared as ``static inline``
> >and can be defined in a .c or a .h file.
> >
> >Const Attribute
> >---------------
> >
> >Particular care must be taken with the use of the ``const`` attribute. It
> >should be used as often as possible when a variable is read-only.
> >
> >ASM Coding Rules
> >----------------
> >
> >Assembly Syntax
> >
> >NASM is used for assembly, with the syntax, therefore guidelines given
> >here are appropriate to this target.
> >[GNU as is intended to support both syntax variants, but that is not
> >documented here]. The following general guidelines are valid in any case.
> > globals, extern and macros are to be defined at the top of the file
> > labels should stay explicit, and are left aligned
> > code is indented with a tabulation, no spaces
> > instruction and operands should be separated by a tab too
> > code should be separated in blocks
> > blocks, when possible, should start with a comment explanation
> >
> >Sample code:
> > ; comment header
> >
> > ; export this symbol
> > [GLOBAL entry]
> >
> > ; external variables and functions
> > [EXTERN variable]
> >
> > ; 16 bits code
> > [BITS 16]
> >
> > ; macros like
> > BIOS_START EQU 0x7C00
> >
> > entry:
> >
> > ; Clear interrupt flag
> > cli
> >
> > ; Set segment registers to 0
> > xor bx, bx
> > mov es, bx
> > mov fs, bx
> > mov gs, bx
> > mov ds, bx
> > mov ss, bx
> > mov sp, 0x7C00
> > sti
> >
> >;; [...] snip [...]
> >
> >
> >Use of C-style macros is allowed. When compiling ASM code, a file is
> >parsed by the C preprocessor. It is then allowed to share some constants
> >between C and assembly code, in a ``.h`` file.
> >
> >Inline ASM in C code
> >
> >The ``asm`` and ``volatile`` keywords do not have underscores. The AT&T
> >syntax should be used. Input and output operands should be named to avoid
> >confusion,
> >as shown in the following example::
> > asm volatile("outb %[val], %[port]"
> > : :
> > [port] "dN" (port),
> > [val] "a" (val));
> >
> >
> >Environment or Architecture-specific Sources
> >
> >In DPDK and DPDK applications, some code is specific to an architecture
> >(i686, x86_64) or to an executive environment (bare-metal or linuxapp)
> >and so on.
> >
> >There are several ways to handle specific code:
> > Use a ``#ifdef`` with the CONFIG option in the C code. This can be done
> >when the differences are small and they can be embedded in the same C
> >file::
> > #ifdef RTE_ARCH_I686
> > toto();
> > #else
> > titi();
> > #endif
> >
> >Use the CONFIG option in the Makefile. This is done when the differences
> >are more significant. In this case, the code is split into
> >two separate files that are architecture or environment specific.
> >
> >The same logic applies to header files.
> >
> >By convention, a file is common if it is not located in a directory
> >indicating that it is specific. For instance, a file located in a
> >subdir of "x86_64" directory is specific to this architecture. A file
> >located in a subdir of "linuxapp" is specific to this execution
> >environment.
> >NOTE As in the linux kernel, the "CONFIG_" prefix is not used in C code.
> >This is only needed in Makefiles or shell scripts.
> >Per Architecture Sources
> >
> >The following config options can be used:
> > CONFIG_RTE_ARCH is a string that contains the name of the architecture.
> > CONFIG_RTE_ARCH_I686, CONFIG_RTE_ARCH_X86_64 or
> >CONFIG_RTE_ARCH_X86_64_32 are defined only if we are building for those
> >architectures.
> >
> >Per Execution Environment Sources
> >
> >The following config options can be used:
> > CONFIG_RTE_EXEC_ENV is a string that contains the name of the executive
> >environment.
> > CONFIG_RTE_EXEC_ENV_BAREMETAL or CONFIG_RTE_EXEC_ENV_LINUXAPP are
> >defined only if we are building for this execution environment.
> >
> >Per Driver Sources
> > RTE_LIBRTE_IGB_PMD or RTE_LIBRTE_IXGBE_PMD are defined only if we are
> >using this driver.
> >
> >Doxygen Documentation
> >---------------------
> >
> >The API documentation is automatically generated in the DPDK framework.
> >That is why all files that are part of the public
> >API must be documented using Doxygen syntax.
> >
> >The public API comprises functions of DPDK that can be used by an
> >external application that will use the SDK. Only the Doxygen
> >syntax described in the coding rules (this document) should be used in
> >the code. All the Doxygen features are described in the Doxygen manual
> >online.
> >
> >Documenting a Function
> >
> >All public functions must be documented. The documentation is placed in
> >the header file, above the declaration of the function.
> >The definition of the function may be documented, but using standard
> >comments (not in doxygen format).
> >Private functions can be documented using Doxygen. The following is an
> >example of function documentation:
> > /**
> > * Summary here; one sentence on one line (should not exceed 80 chars).
> > *
> > * A more detailed description goes here.
> > *
> > * A blank line forms a paragraph. There should be no trailing
> >white-space
> > * anywhere.
> > *
> > * @param first
> > * "@param" is a Doxygen directive to describe a function parameter.
> >Like
> > * some other directives, it takes a term/summary on the same line and
> >a
> > * description (this text) indented by 2 spaces on the next line. All
> > * descriptive text should wrap at 80 chars, without going over.
> > * Newlines are NOT supported within directives; if a newline would be
> > * before this text, it would be appended to the general description
> >above.
> > * @param second
> > * There should be no newline between multiple directives (of the same
> > * type).
> > *
> > * @return
> > * "@return" is a different Doxygen directive to describe the return
> >value
> > * of a function, if there is any.
> > */
> > int rte_foo(int first, int second)
> >
> >
> >Documenting Files
> >
> >Each public file may start with a comment describing what the file does.
> >For example:
> > /**
> > * @file
> > * This file describes the coding rules of RTE.
> > *
> > * It contains the coding rules of C code, ASM code, reStructured
> > * Text documentation, and of course how to use doxygen to document
> > * public API.
> > */
> >
> >
> >Documenting Constants and Variables
> > /**
> > * The definition of a funny TRUE.
> > */
> > #define TRUE 0
> >
> >
> >
> >
> > #define TRUE 1 /**< another way to document a macro */
> >
> > /**
> > * Frequency of the HPET counter in Hz
> > *
> > * @see rte_eal_hpet_init()
> > */
> > extern uint64_t eal_hpet_resolution_hz;
> >
> >
> >Documenting Structures
> >
> >Public structures should also be documented. The ``/**<`` sequence can be
> >used to documented the fields of the structure, as shown in the following
> >example:
> > /**
> > * Structure describing a memzone, which is a contiguous portions of
> > * physical memory identified by a name.
> > */
> > struct rte_memzone {
> >
> > #define MEMZONE_NAMESIZE 32
> > char name[MEMZONE_NAMESIZE]; /**< name of the memory zone */
> >
> > phys_addr_t phys_addr; /**< start physical address */
> > void *addr; /**< start virtual address */
> > uint64_t len; /**< len of the memzone */
> >
> > int socket_id; /**< NUMA socket id */
> > };
> >
> >
> >Using Lists
> >
> >Using the minus character, it is possible to generate a bullet list. The
> >minus signs must be column-aligned. If the minus sign is followed by a
> >hash,
> >then it generates an enumerated list. Refer to the official Doxygen
> >documentation for more information.
> > /**
> > * A list of events:
> > * - mouse events
> > * -# mouse move event
> > * -# mouse click event\n
> > * More info about the click event.
> > * -# mouse double click event
> > * - keyboard events
> > * -# key down event
> > * -# key up event
> > *
> > * More text here.
> > */
> >
> >
> >See Also Sections
> >
> >The @see keyword can be used to highlight a link to an existing function,
> >file, or URL. This directive should be placed on one line, without
> >anything else, at the bottom of the documentation header.
> > /**
> > * (documentation of function, file, ...)
> > *
> > * @see rte_foo()
> > * @see eal_memzone.c
> > */
> >
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
2015-04-08 11:43 ` Neil Horman
2015-04-08 14:16 ` Wiles, Keith
@ 2015-04-08 15:21 ` Wiles, Keith
2015-04-08 15:53 ` Wiles, Keith
` (4 subsequent siblings)
7 siblings, 0 replies; 62+ messages in thread
From: Wiles, Keith @ 2015-04-08 15:21 UTC (permalink / raw)
To: Butler, Siobhan A, Thomas Monjalon, dev
On 4/8/15, 5:43 AM, "Butler, Siobhan A" <siobhan.a.butler@intel.com> wrote:
>Hi all,
>To add to the tools brainstorming - I propose we use the following Coding
>Standards as the basis of guidelines on coding style going forward.
>The style outlined below is in alignment with the current convention used
>for the majority of the project.
>Any thoughts/suggestions or feedback welcome.
>Thanks
>Siobhan :)
><siobhan.a.butler@intel.com>
>
Have we looked at astyle or some code formatter to help get alignment?
>
>
>Coding Style
>~~~~~~~~~~
>
>Description
>-----------
>
>This document specifies the preferred style for source files in the DPDK
>source tree.
>It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
>Kernel Developer's Manual (see man style(9)),
>but was heavily modified for the needs of the DPDK. Many of the style
>rules are implicit in the examples.
>Be careful to check the examples before assuming that style is silent on
>an issue.
>
>General Guidelines
>------------------
>
>The rules and guidelines given in this document cannot cover every
>situation, so the following general guidelines should be used as a
>fallback:
>The code style should be consistent within each individual file, and
>within each file in a given directory or module - in the case of creating
>new files
>The primary reason for coding standards is to increase code readability
>and comprehensibility, therefore always use whatever option will make the
>code easiest to read.
>
>The following more specific recommendations apply to all sections, both
>for C and assembly code:
>Line length is recommended to be not more than 80 characters, including
>comments. [Tab stop size should be assumed to be at least 4-characters
>wide]
>Indentation should be to no more than 3 levels deep.
>NOTE The above are recommendations, and not hard limits. However, it is
>expected that the recommendations should be followed in all but the
>rarest situations.
>C Comment Style
>
>Usual Comments
>--------------
>
>These comments should be used in normal cases. To document a public API,
>a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
>
>License Header
>--------------
>
>Each file should begin with a special comment tag which will contain the
>appropriate copyright and license for the file (Generally BSD License).
>After any copyright header, a blank line should be left before any other
>contents, e.g. include statements in a C file.
>
>C Preprocessor Directives
>-------------------------
>
>Header Includes
>
>In DPDK sources, the include files should be ordered as following:
> libc includes (system includes first)
> DPDK EAL includes
> DPDK misc libraries includes
> application-specific includes
>
>Example:
> #include <stdio.h>
> #include <stdlib.h>
>
> #include <rte_eal.h>
>
> #include <rte_ring.h>
> #include <rte_mempool.h>
>
> #include "application.h"
>
>
>Global pathnames are defined in <paths.h>. Pathnames local to the program
>go in "pathnames.h" in the local directory.
> #include <paths.h>
>
>
>Leave another blank line before the user include files.
> #include "pathnames.h" /* Local includes in double quotes. */
>
>NOTE Please avoid, as much as possible, including headers from other
>headers file. Doing so should be properly explained and justified.
>Headers should be protected against multiple inclusion with the usual:
> #ifndef _FILE_H_
> #define _FILE_H_
>
> /* Code */
>
> #endif /* _FILE_H_ */
>
>
>Macros
>
>Do not ``#define`` or declare names in the implementation namespace
>except for implementing application interfaces.
>
>The names of ``unsafe`` macros (ones that have side effects), and the
>names of macros for manifest constants, are all in uppercase.
>
>The expansions of expression-like macros are either a single token or
>have outer parentheses. If a macro is an inline expansion of a function,
>the function name is all in lowercase and the macro has the same name all
>in uppercase. Right-justify the backslashes;
>it makes it easier to read. If the macro encapsulates a compound
>statement, enclose it in a do loop, so that it can be used safely in if
>statements.
>Any final statement-terminating semicolon should be supplied by the macro
>invocation rather than the macro, to make parsing easier for
>pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
>
>NOTE Wherever possible, enums and typedefs should be preferred to macros,
>since they provide additional degrees
>of type-safety and can allow compilers to emit extra warnings about
>unsafe code.
>
>Conditional Compilation
>-----------------------
>
>When code is conditionally compiled using #ifdef or #if, a comment may be
>added following the matching #endif or #else to
>permit the reader to easily discern where conditionally compiled code
>regions end. This comment should be used only for
>(subjectively) long regions, regions greater than 20 lines, or where a
>series of nested #ifdef 's may be confusing to the reader.
>Exceptions may be made for cases where code is conditionally not compiled
>for the purposes of lint(1), even though the uncompiled
>region may be small. The comment should be separated from the #endif or
>#else by a single space. For short conditionally compiled regions,
>a closing comment should not be used.
>
>The comment for #endif should match the expression used in the
>corresponding #if or #ifdef. The comment for #else and #elif
>should match the inverse of the expression(s) used in the preceding #if
>and/or #elif statements. In the comments,
>the subexpression defined(FOO) is abbreviated as FOO. For the purposes of
>comments, #ifndef FOO is treated as #if !defined(FOO).
> #ifdef KTRACE
> #include <sys/ktrace.h>
> #endif
>
> #ifdef COMPAT_43
> /* A large region here, or other conditional code. */
> #else /* !COMPAT_43 */
> /* Or here. */
> #endif /* COMPAT_43 */
>
> #ifndef COMPAT_43
> /* Yet another large region here, or other conditional code. */
> #else /* COMPAT_43 */
> /* Or here. */
> #endif /* !COMPAT_43 */
>
>NOTE Conditional compilation should be used only when absolutely
>necessary, as it increases the number of target binaries that need to be
>built and tested.
>C Types
>
>Integers
>
>For fixed/minimum-size integer values, the project uses the form uintXX_t
>(from stdint.h) instead of older BSD-style integer identifiers of the
>form u_intXX_t.
>
>Enumerations
>------------
>
>Enumeration values are all uppercase.
> enum enumtype { ONE, TWO } et;
>
>
>Bitfields
>---------
>
>The developer should group bitfields that are included in the same
>integer, as follows:
> struct grehdr {
> uint16_t rec:3,
> srr:1,
> seq:1,
> key:1,
> routing:1,
> csum:1,
> version:3,
> reserved:4,
> ack:1;
> /* ... */
> }
>
>
>Variable Declarations
>---------------------
>
>In declarations, do not put any whitespace between asterisks and adjacent
>tokens, except for tokens that are identifiers related to types.
>(These identifiers are the names of basic types, type qualifiers, and
>typedef-names other than the one being declared.)
>Separate these identifiers from asterisks using a single space.
>
>Structure Declarations
>
>When declaring variables in structures, declare them sorted by use, then
>by size (largest to smallest), and then in alphabetical order.
>Alignment constraints may override the previous rules. The first category
>normally does not apply, but there are exceptions.
>Each structure element gets its own line. Try to make the structure
>readable by aligning the member names using spaces as shown below.
>Names following extremely long types, which therefore cannot be easily
>aligned with the rest, should be separated by a single space.
> struct foo {
> struct foo *next; /* List of active foo. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
>
>
>Major structures should be declared at the top of the file in which they
>are used, or in separate header files if they are used
>in multiple source files. Use of the structures should be by separate
>declarations and should be extern if they are declared in a header file.
>
>Queues
>
>Use queue(3) macros rather than rolling your own lists, whenever
>possible. Thus, the previous example would be better written:
> #include <sys/queue.h>
>
> struct foo {
> LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
> LIST_HEAD(, foo) foohead; /* Head of global foo list. */
>
>
>DPDK also provides an optimized way to store elements in lockless rings.
>This should be used in all data-path code, when there are several
>consumer and/or producers to avoid locking for concurrent access.
>
>Typedefs
>
>Avoid using typedefs for structure types. For example, use:
> struct my_struct_type {
> /* ... */
> };
>
> struct my_struct_type my_var;
>
>
>rather than:
> typedef struct my_struct_type {
> /* ... */
> } my_struct_type;
>
> my_struct_type my_var
>
>
>Typedefs are problematic because they do not properly hide their
>underlying type; for example, you need to know if the typedef is
>the structure itself, as shown above, or a pointer to the structure. In
>addition, they must be declared exactly once, whereas an
>incomplete structure type can be mentioned as many times as necessary.
>Typedefs are difficult to use in stand-alone header files.
>The header that defines the typedef must be included before the header
>that uses it, or by the header that uses it (which causes namespace
>pollution),
>or there must be a back-door mechanism for obtaining the typedef.
>NOTE #defines used instead of typedefs also are problematic (since they
>do not propagate the pointer type correctly due to direct text
>replacement).
>For example, ``#define pint int *`` does not work as expected, while
>``typedef int *pint`` does work. As stated when discussing macros,
>typedefs
>should be preferred to macros in cases like this.
>When convention requires a typedef; make its name match the struct tag.
>Avoid typedefs ending in ``_t``, except as specified in Standard C or by
>POSIX.
> /* Make the structure name match the typedef. */
> typedef struct bar {
> int level;
> } BAR;
> typedef int foo; /* This is foo. */
> typedef const long baz; /* This is baz. */
>
>
>C Function Definition, Declaration and Use
>
>Prototypes
>
>It is recommended, but not required that all functions are prototyped
>somewhere.
>
>Any function prototypes for private functions (that is, functions not
>used elsewhere) go at the top of the first source module. Functions
>local to one source module should be declared static.
>
>Functions used from other parts of code (external API) must be prototyped
>in the relevant include file.
>Function prototypes should be listed in a logical order, preferably
>alphabetical unless there is a compelling reason to use a different
>ordering.
>
>Functions that are used locally in more than one module go into a
>separate header file, for example, "extern.h".
>
>Do not use the ``__P`` macro.
>
>Functions that are part of an external API should be documented using
>Doxygen-like comments above declarations. See the Doxgen documentation
>topic for details.
>
>Associate names with parameter types, for example:
> void function(int fd);
>
>
>Short function prototypes should be contained on a single line. Longer
>prototypes, e.g. those with many parameters,
>can be split across multiple lines. Multi-line prototypes should use
>space-indentation to enable function parameters to line up:
> static char *function1(int _arg, const char *_arg2,
> struct foo *_arg3,
> struct bar *_arg4,
> struct baz *_arg5);
> static void usage(void);
>
>
>Definitions
>-----------
>
>The function type should be on a line by itself preceding the function.
>The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
>
>
>Do not declare functions inside other functions. ANSI C states that such
>declarations have file scope regardless of the nesting of the
>declaration.
>Hiding file declarations in what appears to be a local scope is
>undesirable and will elicit complaints from a good compiler.
>
>Old-style (K&R) function declaration should not be used, use ANSI
>function declarations instead as shown below. Long argument lists
>should be wrapped as described above in the function prototypes section.
> /*
> * All major routines should have a comment briefly describing what
> * they do. The comment before the "main" routine should describe
> * what the program does.
> */
> int
> main(int argc, char *argv[])
> {
> char *ep;
> long num;
> int ch;
>
>
>C Command Line Parsing
>----------------------
>
>For consistency, getopt(3) should be used to parse options. Options
>should be sorted in the getopt(3) call and the switch statement,
>unless parts of the switch cascade. Elements in a switch statement that
>cascade should have a FALLTHROUGH comment.
>Numerical arguments should be checked for accuracy. Code that cannot be
>reached should have a NOTREACHED comment.
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
> switch (ch) { /* Indent the switch. */
> case 'a': /* Don't indent the case. */
> aflag = 1; /* Indent case body one tab. */
> /* FALLTHROUGH */
> case 'b':
> bflag = 1;
> break;
> case 'N':
> Nflag = 1;
> break;
> case 'n':
> num = strtol(optarg, &ep, 10);
> if (num <= 0 || *ep != '\0') {
> warnx("illegal number, -n argument -- %s",
> optarg);
> usage();
> }
> break;
> case '?':
> default:
> usage();
> /* NOTREACHED */
> }
> argc -= optind;
> argv += optind;
>
>
>
>
>
>C Indentation
>-------------
>
>Control Statements and Loops
>
>Include a space after keywords (if, while, for, return, switch). Do not
>use braces (``{`` and ``}``) for control statements with zero or just a
>single statement, unless that statement is more than a single line in
>which case the braces are permitted. Forever loops are done with for
>statements, not while statements.
> for (p = buf; *p != '\0'; ++p)
> ; /* nothing */
> for (;;)
> stmt;
> for (;;) {
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
> }
> for (;;) {
> if (cond)
> stmt;
> }
> if (val != NULL)
> val = realloc(val, newsize);
>
>
>Parts of a for loop may be left empty. It is recommended that you do not
>put declarations inside blocks unless the routine is unusually
>complicated.
> for (; cnt < 15; cnt++) {
> stmt1;
> stmt2;
> }
>
>
>Indentation is a hard tab, that is, a tab character, not a sequence of
>spaces.
>NOTE General rule in DPDK, use tabs for indentation, spaces for
>alignment.
>If you have to wrap a long statement, put the operator at the end of the
>line, and indent again. For control statements (if, while, etc.),
>it is recommended that the next line be indented by two tabs, rather than
>one, to prevent confusion as to whether the second line of the
>control statement forms part of the statement body or not. For
>non-control statements, this issue does not apply, so they can be
>indented
>by a single tab. However, a two-tab indent is recommended in this case
>also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
>Do not add whitespace at the end of a line.
>
>Closing and opening braces go on the same line as the else keyword.
>Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
>Function Calls
>--------------
>
>Do not use spaces after function names. Commas should have a space after
>them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)``
>characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
>
>
>Operators
>---------
>
>Unary operators do not require spaces, binary operators do. Do not use
>parentheses unless they are required for precedence or unless the
>statement is confusing without them. Remember that other people may be
>more easily confused than you.
>
>Exit
>
>Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
> }
>
>
>Local Variables
>---------------
>
>When declaring variables in functions, declare them sorted by size, then
>in alphabetical order. Multiple variables per line are OK.
>If a line overflows reuse the type keyword.
>
>Be careful to not obfuscate the code by initializing variables in the
>declarations, only the last variable on a line should be initialized.
>If multiple variables are to be initialised when defined, put one per
>line. Do not use function calls in initializers.
> int i = 0, j = 0, k = 0; /* bad, too many initializer */
>
> char a = 0; /* OK, one variable per line with initializer */
> char b = 0;
>
> float x, y = 0.0; /* OK, only last variable has initializer */
>
>
>Casts and sizeof
>
>Casts and sizeof statements are not followed by a space. Always write
>sizeof statements with parenthesis.
>The redundant parenthesis rules do not apply to sizeof(var) instances.
>
>C Style and Conventions
>
>NULL Pointers
>
>NULL is the preferred null pointer constant. Use NULL instead of ``(type
>*)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
>for example, in assignments. Use ``(type *)NULL`` in other contexts, in
>particular for all function args.
>(Casting is essential for variadic args and is necessary for other args
>if the function prototype might not be in scope.) Test pointers against
>NULL, for example, use::
> (p = f()) == NULL
>
>
>not::
> !(p = f())
>
>
>Do not use ! for tests unless it is a boolean, for example, use::
> if (*p == '\0')
>
>
>not::
> if (!*p)
>
>
>Return Value
>------------
>
>If possible, functions should return 0 on success and a negative value on
>error. The negative value should be ``-errno`` if relevant, for example,
>``-EINVAL``.
>
>Routines returning ``void *`` should not have their return values cast to
>any pointer type.
>(Typecasting can prevent the compiler from warning about missing
>prototypes as any implicit definition of a function returns int - which,
>unlike "void *" needs a typecast to assign to a pointer variable.)
>NOTE The above rule applies to malloc, as well as to DPDK functions.
>Values in return statements should be enclosed in parentheses.
>
>Logging and Errors
>------------------
>
>In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
>
>In a userland program that is not a DPDK application, use err(3) or
>warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
> }
>
>
>Variable Arguments List
>-----------------------
>
>Variable numbers of arguments should look like this:
> #include <stdarg.h>
>
> void
> vaf(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> STUFF;
> va_end(ap);
> /* No return needed for void functions. */
> }
>
> static void
> usage()
> {
> /* Insert an empty line if the function has no local variables.
>*/
>
>
>Printf
>------
>
>Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is
>faster and usually cleaner, and helps to avoid unnecessary bugs. However,
>be aware of format string bugs::
> int
> main(int argc, char **argv)
> {
> if(argc != 2)
> exit(1);
> printf(argv[1]); /* bad ! */
> printf("%s", argv[1]); /* correct */
>
>
>Usage
>-----
>
>Usage statements should look like the manual pages SYNOPSIS. The usage
>statement should be structured in the following order:
>1. Options without operands come first, in alphabetical order, inside a
>single set of brackets (``[`` and ``]``).
>2. Options with operands come next, also in alphabetical order, with each
>option and its argument inside its own pair of brackets.
>3. Required arguments (if any) are next, listed in the order they should
>be specified on the command line.
>4. Finally, any optional arguments, listed in the order they should be
>specified, and all inside brackets.
>
>A bar (`|') separates ``either-or`` options/arguments, and multiple
>options/arguments, which are specified together, are placed in a single
>set of brackets.
> "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
>
> (void)fprintf(stderr, "usage: f [-ab]\n");
> exit(1);
> }
>
>
>Note that the manual page options description should list the options in
>pure alphabetical order. That is, without regard to
>whether an option takes arguments or not. The alphabetical ordering
>should take into account the case ordering shown above.
>
>Branch Prediction
>-----------------
>
>When a test is done in a critical zone (called often or in a data path)
>use the ``likely()`` and ``unlikely()`` macros. They are expanded
>as a compiler builtin and allow the developer to indicate if the branch
>is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
>
>
>Static Variables and Functions
>------------------------------
>
>All functions and variables that are local to a file must be declared as
>``static`` because it can often help the compiler to do
>some optimizations (such as, inlining the code).
>
>Functions that must be inlined have to be declared as ``static inline``
>and can be defined in a .c or a .h file.
>
>Const Attribute
>---------------
>
>Particular care must be taken with the use of the ``const`` attribute. It
>should be used as often as possible when a variable is read-only.
>
>ASM Coding Rules
>----------------
>
>Assembly Syntax
>
>NASM is used for assembly, with the syntax, therefore guidelines given
>here are appropriate to this target.
>[GNU as is intended to support both syntax variants, but that is not
>documented here]. The following general guidelines are valid in any case.
> globals, extern and macros are to be defined at the top of the file
> labels should stay explicit, and are left aligned
> code is indented with a tabulation, no spaces
> instruction and operands should be separated by a tab too
> code should be separated in blocks
> blocks, when possible, should start with a comment explanation
>
>Sample code:
> ; comment header
>
> ; export this symbol
> [GLOBAL entry]
>
> ; external variables and functions
> [EXTERN variable]
>
> ; 16 bits code
> [BITS 16]
>
> ; macros like
> BIOS_START EQU 0x7C00
>
> entry:
>
> ; Clear interrupt flag
> cli
>
> ; Set segment registers to 0
> xor bx, bx
> mov es, bx
> mov fs, bx
> mov gs, bx
> mov ds, bx
> mov ss, bx
> mov sp, 0x7C00
> sti
>
>;; [...] snip [...]
>
>
>Use of C-style macros is allowed. When compiling ASM code, a file is
>parsed by the C preprocessor. It is then allowed to share some constants
>between C and assembly code, in a ``.h`` file.
>
>Inline ASM in C code
>
>The ``asm`` and ``volatile`` keywords do not have underscores. The AT&T
>syntax should be used. Input and output operands should be named to avoid
>confusion,
>as shown in the following example::
> asm volatile("outb %[val], %[port]"
> : :
> [port] "dN" (port),
> [val] "a" (val));
>
>
>Environment or Architecture-specific Sources
>
>In DPDK and DPDK applications, some code is specific to an architecture
>(i686, x86_64) or to an executive environment (bare-metal or linuxapp)
>and so on.
>
>There are several ways to handle specific code:
> Use a ``#ifdef`` with the CONFIG option in the C code. This can be done
>when the differences are small and they can be embedded in the same C
>file::
> #ifdef RTE_ARCH_I686
> toto();
> #else
> titi();
> #endif
>
>Use the CONFIG option in the Makefile. This is done when the differences
>are more significant. In this case, the code is split into
>two separate files that are architecture or environment specific.
>
>The same logic applies to header files.
>
>By convention, a file is common if it is not located in a directory
>indicating that it is specific. For instance, a file located in a
>subdir of "x86_64" directory is specific to this architecture. A file
>located in a subdir of "linuxapp" is specific to this execution
>environment.
>NOTE As in the linux kernel, the "CONFIG_" prefix is not used in C code.
>This is only needed in Makefiles or shell scripts.
>Per Architecture Sources
>
>The following config options can be used:
> CONFIG_RTE_ARCH is a string that contains the name of the architecture.
> CONFIG_RTE_ARCH_I686, CONFIG_RTE_ARCH_X86_64 or
>CONFIG_RTE_ARCH_X86_64_32 are defined only if we are building for those
>architectures.
>
>Per Execution Environment Sources
>
>The following config options can be used:
> CONFIG_RTE_EXEC_ENV is a string that contains the name of the executive
>environment.
> CONFIG_RTE_EXEC_ENV_BAREMETAL or CONFIG_RTE_EXEC_ENV_LINUXAPP are
>defined only if we are building for this execution environment.
>
>Per Driver Sources
> RTE_LIBRTE_IGB_PMD or RTE_LIBRTE_IXGBE_PMD are defined only if we are
>using this driver.
>
>Doxygen Documentation
>---------------------
>
>The API documentation is automatically generated in the DPDK framework.
>That is why all files that are part of the public
>API must be documented using Doxygen syntax.
>
>The public API comprises functions of DPDK that can be used by an
>external application that will use the SDK. Only the Doxygen
>syntax described in the coding rules (this document) should be used in
>the code. All the Doxygen features are described in the Doxygen manual
>online.
>
>Documenting a Function
>
>All public functions must be documented. The documentation is placed in
>the header file, above the declaration of the function.
>The definition of the function may be documented, but using standard
>comments (not in doxygen format).
>Private functions can be documented using Doxygen. The following is an
>example of function documentation:
> /**
> * Summary here; one sentence on one line (should not exceed 80 chars).
> *
> * A more detailed description goes here.
> *
> * A blank line forms a paragraph. There should be no trailing
>white-space
> * anywhere.
> *
> * @param first
> * "@param" is a Doxygen directive to describe a function parameter.
>Like
> * some other directives, it takes a term/summary on the same line and
>a
> * description (this text) indented by 2 spaces on the next line. All
> * descriptive text should wrap at 80 chars, without going over.
> * Newlines are NOT supported within directives; if a newline would be
> * before this text, it would be appended to the general description
>above.
> * @param second
> * There should be no newline between multiple directives (of the same
> * type).
> *
> * @return
> * "@return" is a different Doxygen directive to describe the return
>value
> * of a function, if there is any.
> */
> int rte_foo(int first, int second)
>
>
>Documenting Files
>
>Each public file may start with a comment describing what the file does.
>For example:
> /**
> * @file
> * This file describes the coding rules of RTE.
> *
> * It contains the coding rules of C code, ASM code, reStructured
> * Text documentation, and of course how to use doxygen to document
> * public API.
> */
>
>
>Documenting Constants and Variables
> /**
> * The definition of a funny TRUE.
> */
> #define TRUE 0
>
>
>
>
> #define TRUE 1 /**< another way to document a macro */
>
> /**
> * Frequency of the HPET counter in Hz
> *
> * @see rte_eal_hpet_init()
> */
> extern uint64_t eal_hpet_resolution_hz;
>
>
>Documenting Structures
>
>Public structures should also be documented. The ``/**<`` sequence can be
>used to documented the fields of the structure, as shown in the following
>example:
> /**
> * Structure describing a memzone, which is a contiguous portions of
> * physical memory identified by a name.
> */
> struct rte_memzone {
>
> #define MEMZONE_NAMESIZE 32
> char name[MEMZONE_NAMESIZE]; /**< name of the memory zone */
>
> phys_addr_t phys_addr; /**< start physical address */
> void *addr; /**< start virtual address */
> uint64_t len; /**< len of the memzone */
>
> int socket_id; /**< NUMA socket id */
> };
>
>
>Using Lists
>
>Using the minus character, it is possible to generate a bullet list. The
>minus signs must be column-aligned. If the minus sign is followed by a
>hash,
>then it generates an enumerated list. Refer to the official Doxygen
>documentation for more information.
> /**
> * A list of events:
> * - mouse events
> * -# mouse move event
> * -# mouse click event\n
> * More info about the click event.
> * -# mouse double click event
> * - keyboard events
> * -# key down event
> * -# key up event
> *
> * More text here.
> */
>
>
>See Also Sections
>
>The @see keyword can be used to highlight a link to an existing function,
>file, or URL. This directive should be placed on one line, without
>anything else, at the bottom of the documentation header.
> /**
> * (documentation of function, file, ...)
> *
> * @see rte_foo()
> * @see eal_memzone.c
> */
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
` (2 preceding siblings ...)
2015-04-08 15:21 ` Wiles, Keith
@ 2015-04-08 15:53 ` Wiles, Keith
2015-04-08 16:16 ` Thomas Monjalon
2015-04-08 18:16 ` Stephen Hemminger
` (3 subsequent siblings)
7 siblings, 1 reply; 62+ messages in thread
From: Wiles, Keith @ 2015-04-08 15:53 UTC (permalink / raw)
To: Butler, Siobhan A, Thomas Monjalon, dev
On 4/8/15, 5:43 AM, "Butler, Siobhan A" <siobhan.a.butler@intel.com> wrote:
>Hi all,
>To add to the tools brainstorming - I propose we use the following Coding
>Standards as the basis of guidelines on coding style going forward.
>The style outlined below is in alignment with the current convention used
>for the majority of the project.
>Any thoughts/suggestions or feedback welcome.
>Thanks
>Siobhan :)
><siobhan.a.butler@intel.com>
>
One of the biggest problems with any style is helping the developer
maintain the style. Using some tool does help and I have used astyle
before, not bad code formatter. Here is a few that seem to be reasonable.
http://astyle.sourceforge.net/
http://uncrustify.sourceforge.net/
http://sourceforge.net/projects/gcgreatcode/
A nice visual editor to define the style and runs on a number of
platforms. It seems using this visual tool helps define the config files
for the above tools.
http://universalindent.sourceforge.net/
Maybe more tools, just the few I found.
>
>
>Coding Style
>~~~~~~~~~~
>
>Description
>-----------
>
>This document specifies the preferred style for source files in the DPDK
>source tree.
>It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
>Kernel Developer's Manual (see man style(9)),
>but was heavily modified for the needs of the DPDK. Many of the style
>rules are implicit in the examples.
>Be careful to check the examples before assuming that style is silent on
>an issue.
>
>General Guidelines
>------------------
>
>The rules and guidelines given in this document cannot cover every
>situation, so the following general guidelines should be used as a
>fallback:
>The code style should be consistent within each individual file, and
>within each file in a given directory or module - in the case of creating
>new files
>The primary reason for coding standards is to increase code readability
>and comprehensibility, therefore always use whatever option will make the
>code easiest to read.
>
>The following more specific recommendations apply to all sections, both
>for C and assembly code:
>Line length is recommended to be not more than 80 characters, including
>comments. [Tab stop size should be assumed to be at least 4-characters
>wide]
>Indentation should be to no more than 3 levels deep.
>NOTE The above are recommendations, and not hard limits. However, it is
>expected that the recommendations should be followed in all but the
>rarest situations.
>C Comment Style
>
>Usual Comments
>--------------
>
>These comments should be used in normal cases. To document a public API,
>a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
>
>License Header
>--------------
>
>Each file should begin with a special comment tag which will contain the
>appropriate copyright and license for the file (Generally BSD License).
>After any copyright header, a blank line should be left before any other
>contents, e.g. include statements in a C file.
>
>C Preprocessor Directives
>-------------------------
>
>Header Includes
>
>In DPDK sources, the include files should be ordered as following:
> libc includes (system includes first)
> DPDK EAL includes
> DPDK misc libraries includes
> application-specific includes
>
>Example:
> #include <stdio.h>
> #include <stdlib.h>
>
> #include <rte_eal.h>
>
> #include <rte_ring.h>
> #include <rte_mempool.h>
>
> #include "application.h"
>
>
>Global pathnames are defined in <paths.h>. Pathnames local to the program
>go in "pathnames.h" in the local directory.
> #include <paths.h>
>
>
>Leave another blank line before the user include files.
> #include "pathnames.h" /* Local includes in double quotes. */
>
>NOTE Please avoid, as much as possible, including headers from other
>headers file. Doing so should be properly explained and justified.
>Headers should be protected against multiple inclusion with the usual:
> #ifndef _FILE_H_
> #define _FILE_H_
>
> /* Code */
>
> #endif /* _FILE_H_ */
>
>
>Macros
>
>Do not ``#define`` or declare names in the implementation namespace
>except for implementing application interfaces.
>
>The names of ``unsafe`` macros (ones that have side effects), and the
>names of macros for manifest constants, are all in uppercase.
>
>The expansions of expression-like macros are either a single token or
>have outer parentheses. If a macro is an inline expansion of a function,
>the function name is all in lowercase and the macro has the same name all
>in uppercase. Right-justify the backslashes;
>it makes it easier to read. If the macro encapsulates a compound
>statement, enclose it in a do loop, so that it can be used safely in if
>statements.
>Any final statement-terminating semicolon should be supplied by the macro
>invocation rather than the macro, to make parsing easier for
>pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
>
>NOTE Wherever possible, enums and typedefs should be preferred to macros,
>since they provide additional degrees
>of type-safety and can allow compilers to emit extra warnings about
>unsafe code.
>
>Conditional Compilation
>-----------------------
>
>When code is conditionally compiled using #ifdef or #if, a comment may be
>added following the matching #endif or #else to
>permit the reader to easily discern where conditionally compiled code
>regions end. This comment should be used only for
>(subjectively) long regions, regions greater than 20 lines, or where a
>series of nested #ifdef 's may be confusing to the reader.
>Exceptions may be made for cases where code is conditionally not compiled
>for the purposes of lint(1), even though the uncompiled
>region may be small. The comment should be separated from the #endif or
>#else by a single space. For short conditionally compiled regions,
>a closing comment should not be used.
>
>The comment for #endif should match the expression used in the
>corresponding #if or #ifdef. The comment for #else and #elif
>should match the inverse of the expression(s) used in the preceding #if
>and/or #elif statements. In the comments,
>the subexpression defined(FOO) is abbreviated as FOO. For the purposes of
>comments, #ifndef FOO is treated as #if !defined(FOO).
> #ifdef KTRACE
> #include <sys/ktrace.h>
> #endif
>
> #ifdef COMPAT_43
> /* A large region here, or other conditional code. */
> #else /* !COMPAT_43 */
> /* Or here. */
> #endif /* COMPAT_43 */
>
> #ifndef COMPAT_43
> /* Yet another large region here, or other conditional code. */
> #else /* COMPAT_43 */
> /* Or here. */
> #endif /* !COMPAT_43 */
>
>NOTE Conditional compilation should be used only when absolutely
>necessary, as it increases the number of target binaries that need to be
>built and tested.
>C Types
>
>Integers
>
>For fixed/minimum-size integer values, the project uses the form uintXX_t
>(from stdint.h) instead of older BSD-style integer identifiers of the
>form u_intXX_t.
>
>Enumerations
>------------
>
>Enumeration values are all uppercase.
> enum enumtype { ONE, TWO } et;
>
>
>Bitfields
>---------
>
>The developer should group bitfields that are included in the same
>integer, as follows:
> struct grehdr {
> uint16_t rec:3,
> srr:1,
> seq:1,
> key:1,
> routing:1,
> csum:1,
> version:3,
> reserved:4,
> ack:1;
> /* ... */
> }
>
>
>Variable Declarations
>---------------------
>
>In declarations, do not put any whitespace between asterisks and adjacent
>tokens, except for tokens that are identifiers related to types.
>(These identifiers are the names of basic types, type qualifiers, and
>typedef-names other than the one being declared.)
>Separate these identifiers from asterisks using a single space.
>
>Structure Declarations
>
>When declaring variables in structures, declare them sorted by use, then
>by size (largest to smallest), and then in alphabetical order.
>Alignment constraints may override the previous rules. The first category
>normally does not apply, but there are exceptions.
>Each structure element gets its own line. Try to make the structure
>readable by aligning the member names using spaces as shown below.
>Names following extremely long types, which therefore cannot be easily
>aligned with the rest, should be separated by a single space.
> struct foo {
> struct foo *next; /* List of active foo. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
>
>
>Major structures should be declared at the top of the file in which they
>are used, or in separate header files if they are used
>in multiple source files. Use of the structures should be by separate
>declarations and should be extern if they are declared in a header file.
>
>Queues
>
>Use queue(3) macros rather than rolling your own lists, whenever
>possible. Thus, the previous example would be better written:
> #include <sys/queue.h>
>
> struct foo {
> LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members
>*/
> };
> LIST_HEAD(, foo) foohead; /* Head of global foo list. */
>
>
>DPDK also provides an optimized way to store elements in lockless rings.
>This should be used in all data-path code, when there are several
>consumer and/or producers to avoid locking for concurrent access.
>
>Typedefs
>
>Avoid using typedefs for structure types. For example, use:
> struct my_struct_type {
> /* ... */
> };
>
> struct my_struct_type my_var;
>
>
>rather than:
> typedef struct my_struct_type {
> /* ... */
> } my_struct_type;
>
> my_struct_type my_var
>
>
>Typedefs are problematic because they do not properly hide their
>underlying type; for example, you need to know if the typedef is
>the structure itself, as shown above, or a pointer to the structure. In
>addition, they must be declared exactly once, whereas an
>incomplete structure type can be mentioned as many times as necessary.
>Typedefs are difficult to use in stand-alone header files.
>The header that defines the typedef must be included before the header
>that uses it, or by the header that uses it (which causes namespace
>pollution),
>or there must be a back-door mechanism for obtaining the typedef.
>NOTE #defines used instead of typedefs also are problematic (since they
>do not propagate the pointer type correctly due to direct text
>replacement).
>For example, ``#define pint int *`` does not work as expected, while
>``typedef int *pint`` does work. As stated when discussing macros,
>typedefs
>should be preferred to macros in cases like this.
>When convention requires a typedef; make its name match the struct tag.
>Avoid typedefs ending in ``_t``, except as specified in Standard C or by
>POSIX.
> /* Make the structure name match the typedef. */
> typedef struct bar {
> int level;
> } BAR;
> typedef int foo; /* This is foo. */
> typedef const long baz; /* This is baz. */
>
>
>C Function Definition, Declaration and Use
>
>Prototypes
>
>It is recommended, but not required that all functions are prototyped
>somewhere.
>
>Any function prototypes for private functions (that is, functions not
>used elsewhere) go at the top of the first source module. Functions
>local to one source module should be declared static.
>
>Functions used from other parts of code (external API) must be prototyped
>in the relevant include file.
>Function prototypes should be listed in a logical order, preferably
>alphabetical unless there is a compelling reason to use a different
>ordering.
>
>Functions that are used locally in more than one module go into a
>separate header file, for example, "extern.h".
>
>Do not use the ``__P`` macro.
>
>Functions that are part of an external API should be documented using
>Doxygen-like comments above declarations. See the Doxgen documentation
>topic for details.
>
>Associate names with parameter types, for example:
> void function(int fd);
>
>
>Short function prototypes should be contained on a single line. Longer
>prototypes, e.g. those with many parameters,
>can be split across multiple lines. Multi-line prototypes should use
>space-indentation to enable function parameters to line up:
> static char *function1(int _arg, const char *_arg2,
> struct foo *_arg3,
> struct bar *_arg4,
> struct baz *_arg5);
> static void usage(void);
>
>
>Definitions
>-----------
>
>The function type should be on a line by itself preceding the function.
>The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
>
>
>Do not declare functions inside other functions. ANSI C states that such
>declarations have file scope regardless of the nesting of the
>declaration.
>Hiding file declarations in what appears to be a local scope is
>undesirable and will elicit complaints from a good compiler.
>
>Old-style (K&R) function declaration should not be used, use ANSI
>function declarations instead as shown below. Long argument lists
>should be wrapped as described above in the function prototypes section.
> /*
> * All major routines should have a comment briefly describing what
> * they do. The comment before the "main" routine should describe
> * what the program does.
> */
> int
> main(int argc, char *argv[])
> {
> char *ep;
> long num;
> int ch;
>
>
>C Command Line Parsing
>----------------------
>
>For consistency, getopt(3) should be used to parse options. Options
>should be sorted in the getopt(3) call and the switch statement,
>unless parts of the switch cascade. Elements in a switch statement that
>cascade should have a FALLTHROUGH comment.
>Numerical arguments should be checked for accuracy. Code that cannot be
>reached should have a NOTREACHED comment.
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
> switch (ch) { /* Indent the switch. */
> case 'a': /* Don't indent the case. */
> aflag = 1; /* Indent case body one tab. */
> /* FALLTHROUGH */
> case 'b':
> bflag = 1;
> break;
> case 'N':
> Nflag = 1;
> break;
> case 'n':
> num = strtol(optarg, &ep, 10);
> if (num <= 0 || *ep != '\0') {
> warnx("illegal number, -n argument -- %s",
> optarg);
> usage();
> }
> break;
> case '?':
> default:
> usage();
> /* NOTREACHED */
> }
> argc -= optind;
> argv += optind;
>
>
>
>
>
>C Indentation
>-------------
>
>Control Statements and Loops
>
>Include a space after keywords (if, while, for, return, switch). Do not
>use braces (``{`` and ``}``) for control statements with zero or just a
>single statement, unless that statement is more than a single line in
>which case the braces are permitted. Forever loops are done with for
>statements, not while statements.
> for (p = buf; *p != '\0'; ++p)
> ; /* nothing */
> for (;;)
> stmt;
> for (;;) {
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
> }
> for (;;) {
> if (cond)
> stmt;
> }
> if (val != NULL)
> val = realloc(val, newsize);
>
>
>Parts of a for loop may be left empty. It is recommended that you do not
>put declarations inside blocks unless the routine is unusually
>complicated.
> for (; cnt < 15; cnt++) {
> stmt1;
> stmt2;
> }
>
>
>Indentation is a hard tab, that is, a tab character, not a sequence of
>spaces.
>NOTE General rule in DPDK, use tabs for indentation, spaces for
>alignment.
>If you have to wrap a long statement, put the operator at the end of the
>line, and indent again. For control statements (if, while, etc.),
>it is recommended that the next line be indented by two tabs, rather than
>one, to prevent confusion as to whether the second line of the
>control statement forms part of the statement body or not. For
>non-control statements, this issue does not apply, so they can be
>indented
>by a single tab. However, a two-tab indent is recommended in this case
>also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
>Do not add whitespace at the end of a line.
>
>Closing and opening braces go on the same line as the else keyword.
>Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
>Function Calls
>--------------
>
>Do not use spaces after function names. Commas should have a space after
>them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)``
>characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
>
>
>Operators
>---------
>
>Unary operators do not require spaces, binary operators do. Do not use
>parentheses unless they are required for precedence or unless the
>statement is confusing without them. Remember that other people may be
>more easily confused than you.
>
>Exit
>
>Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
> }
>
>
>Local Variables
>---------------
>
>When declaring variables in functions, declare them sorted by size, then
>in alphabetical order. Multiple variables per line are OK.
>If a line overflows reuse the type keyword.
>
>Be careful to not obfuscate the code by initializing variables in the
>declarations, only the last variable on a line should be initialized.
>If multiple variables are to be initialised when defined, put one per
>line. Do not use function calls in initializers.
> int i = 0, j = 0, k = 0; /* bad, too many initializer */
>
> char a = 0; /* OK, one variable per line with initializer */
> char b = 0;
>
> float x, y = 0.0; /* OK, only last variable has initializer */
>
>
>Casts and sizeof
>
>Casts and sizeof statements are not followed by a space. Always write
>sizeof statements with parenthesis.
>The redundant parenthesis rules do not apply to sizeof(var) instances.
>
>C Style and Conventions
>
>NULL Pointers
>
>NULL is the preferred null pointer constant. Use NULL instead of ``(type
>*)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
>for example, in assignments. Use ``(type *)NULL`` in other contexts, in
>particular for all function args.
>(Casting is essential for variadic args and is necessary for other args
>if the function prototype might not be in scope.) Test pointers against
>NULL, for example, use::
> (p = f()) == NULL
>
>
>not::
> !(p = f())
>
>
>Do not use ! for tests unless it is a boolean, for example, use::
> if (*p == '\0')
>
>
>not::
> if (!*p)
>
>
>Return Value
>------------
>
>If possible, functions should return 0 on success and a negative value on
>error. The negative value should be ``-errno`` if relevant, for example,
>``-EINVAL``.
>
>Routines returning ``void *`` should not have their return values cast to
>any pointer type.
>(Typecasting can prevent the compiler from warning about missing
>prototypes as any implicit definition of a function returns int - which,
>unlike "void *" needs a typecast to assign to a pointer variable.)
>NOTE The above rule applies to malloc, as well as to DPDK functions.
>Values in return statements should be enclosed in parentheses.
>
>Logging and Errors
>------------------
>
>In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
>
>In a userland program that is not a DPDK application, use err(3) or
>warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
> }
>
>
>Variable Arguments List
>-----------------------
>
>Variable numbers of arguments should look like this:
> #include <stdarg.h>
>
> void
> vaf(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> STUFF;
> va_end(ap);
> /* No return needed for void functions. */
> }
>
> static void
> usage()
> {
> /* Insert an empty line if the function has no local variables.
>*/
>
>
>Printf
>------
>
>Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is
>faster and usually cleaner, and helps to avoid unnecessary bugs. However,
>be aware of format string bugs::
> int
> main(int argc, char **argv)
> {
> if(argc != 2)
> exit(1);
> printf(argv[1]); /* bad ! */
> printf("%s", argv[1]); /* correct */
>
>
>Usage
>-----
>
>Usage statements should look like the manual pages SYNOPSIS. The usage
>statement should be structured in the following order:
>1. Options without operands come first, in alphabetical order, inside a
>single set of brackets (``[`` and ``]``).
>2. Options with operands come next, also in alphabetical order, with each
>option and its argument inside its own pair of brackets.
>3. Required arguments (if any) are next, listed in the order they should
>be specified on the command line.
>4. Finally, any optional arguments, listed in the order they should be
>specified, and all inside brackets.
>
>A bar (`|') separates ``either-or`` options/arguments, and multiple
>options/arguments, which are specified together, are placed in a single
>set of brackets.
> "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
>
> (void)fprintf(stderr, "usage: f [-ab]\n");
> exit(1);
> }
>
>
>Note that the manual page options description should list the options in
>pure alphabetical order. That is, without regard to
>whether an option takes arguments or not. The alphabetical ordering
>should take into account the case ordering shown above.
>
>Branch Prediction
>-----------------
>
>When a test is done in a critical zone (called often or in a data path)
>use the ``likely()`` and ``unlikely()`` macros. They are expanded
>as a compiler builtin and allow the developer to indicate if the branch
>is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
>
>
>Static Variables and Functions
>------------------------------
>
>All functions and variables that are local to a file must be declared as
>``static`` because it can often help the compiler to do
>some optimizations (such as, inlining the code).
>
>Functions that must be inlined have to be declared as ``static inline``
>and can be defined in a .c or a .h file.
>
>Const Attribute
>---------------
>
>Particular care must be taken with the use of the ``const`` attribute. It
>should be used as often as possible when a variable is read-only.
>
>ASM Coding Rules
>----------------
>
>Assembly Syntax
>
>NASM is used for assembly, with the syntax, therefore guidelines given
>here are appropriate to this target.
>[GNU as is intended to support both syntax variants, but that is not
>documented here]. The following general guidelines are valid in any case.
> globals, extern and macros are to be defined at the top of the file
> labels should stay explicit, and are left aligned
> code is indented with a tabulation, no spaces
> instruction and operands should be separated by a tab too
> code should be separated in blocks
> blocks, when possible, should start with a comment explanation
>
>Sample code:
> ; comment header
>
> ; export this symbol
> [GLOBAL entry]
>
> ; external variables and functions
> [EXTERN variable]
>
> ; 16 bits code
> [BITS 16]
>
> ; macros like
> BIOS_START EQU 0x7C00
>
> entry:
>
> ; Clear interrupt flag
> cli
>
> ; Set segment registers to 0
> xor bx, bx
> mov es, bx
> mov fs, bx
> mov gs, bx
> mov ds, bx
> mov ss, bx
> mov sp, 0x7C00
> sti
>
>;; [...] snip [...]
>
>
>Use of C-style macros is allowed. When compiling ASM code, a file is
>parsed by the C preprocessor. It is then allowed to share some constants
>between C and assembly code, in a ``.h`` file.
>
>Inline ASM in C code
>
>The ``asm`` and ``volatile`` keywords do not have underscores. The AT&T
>syntax should be used. Input and output operands should be named to avoid
>confusion,
>as shown in the following example::
> asm volatile("outb %[val], %[port]"
> : :
> [port] "dN" (port),
> [val] "a" (val));
>
>
>Environment or Architecture-specific Sources
>
>In DPDK and DPDK applications, some code is specific to an architecture
>(i686, x86_64) or to an executive environment (bare-metal or linuxapp)
>and so on.
>
>There are several ways to handle specific code:
> Use a ``#ifdef`` with the CONFIG option in the C code. This can be done
>when the differences are small and they can be embedded in the same C
>file::
> #ifdef RTE_ARCH_I686
> toto();
> #else
> titi();
> #endif
>
>Use the CONFIG option in the Makefile. This is done when the differences
>are more significant. In this case, the code is split into
>two separate files that are architecture or environment specific.
>
>The same logic applies to header files.
>
>By convention, a file is common if it is not located in a directory
>indicating that it is specific. For instance, a file located in a
>subdir of "x86_64" directory is specific to this architecture. A file
>located in a subdir of "linuxapp" is specific to this execution
>environment.
>NOTE As in the linux kernel, the "CONFIG_" prefix is not used in C code.
>This is only needed in Makefiles or shell scripts.
>Per Architecture Sources
>
>The following config options can be used:
> CONFIG_RTE_ARCH is a string that contains the name of the architecture.
> CONFIG_RTE_ARCH_I686, CONFIG_RTE_ARCH_X86_64 or
>CONFIG_RTE_ARCH_X86_64_32 are defined only if we are building for those
>architectures.
>
>Per Execution Environment Sources
>
>The following config options can be used:
> CONFIG_RTE_EXEC_ENV is a string that contains the name of the executive
>environment.
> CONFIG_RTE_EXEC_ENV_BAREMETAL or CONFIG_RTE_EXEC_ENV_LINUXAPP are
>defined only if we are building for this execution environment.
>
>Per Driver Sources
> RTE_LIBRTE_IGB_PMD or RTE_LIBRTE_IXGBE_PMD are defined only if we are
>using this driver.
>
>Doxygen Documentation
>---------------------
>
>The API documentation is automatically generated in the DPDK framework.
>That is why all files that are part of the public
>API must be documented using Doxygen syntax.
>
>The public API comprises functions of DPDK that can be used by an
>external application that will use the SDK. Only the Doxygen
>syntax described in the coding rules (this document) should be used in
>the code. All the Doxygen features are described in the Doxygen manual
>online.
>
>Documenting a Function
>
>All public functions must be documented. The documentation is placed in
>the header file, above the declaration of the function.
>The definition of the function may be documented, but using standard
>comments (not in doxygen format).
>Private functions can be documented using Doxygen. The following is an
>example of function documentation:
> /**
> * Summary here; one sentence on one line (should not exceed 80 chars).
> *
> * A more detailed description goes here.
> *
> * A blank line forms a paragraph. There should be no trailing
>white-space
> * anywhere.
> *
> * @param first
> * "@param" is a Doxygen directive to describe a function parameter.
>Like
> * some other directives, it takes a term/summary on the same line and
>a
> * description (this text) indented by 2 spaces on the next line. All
> * descriptive text should wrap at 80 chars, without going over.
> * Newlines are NOT supported within directives; if a newline would be
> * before this text, it would be appended to the general description
>above.
> * @param second
> * There should be no newline between multiple directives (of the same
> * type).
> *
> * @return
> * "@return" is a different Doxygen directive to describe the return
>value
> * of a function, if there is any.
> */
> int rte_foo(int first, int second)
>
>
>Documenting Files
>
>Each public file may start with a comment describing what the file does.
>For example:
> /**
> * @file
> * This file describes the coding rules of RTE.
> *
> * It contains the coding rules of C code, ASM code, reStructured
> * Text documentation, and of course how to use doxygen to document
> * public API.
> */
>
>
>Documenting Constants and Variables
> /**
> * The definition of a funny TRUE.
> */
> #define TRUE 0
>
>
>
>
> #define TRUE 1 /**< another way to document a macro */
>
> /**
> * Frequency of the HPET counter in Hz
> *
> * @see rte_eal_hpet_init()
> */
> extern uint64_t eal_hpet_resolution_hz;
>
>
>Documenting Structures
>
>Public structures should also be documented. The ``/**<`` sequence can be
>used to documented the fields of the structure, as shown in the following
>example:
> /**
> * Structure describing a memzone, which is a contiguous portions of
> * physical memory identified by a name.
> */
> struct rte_memzone {
>
> #define MEMZONE_NAMESIZE 32
> char name[MEMZONE_NAMESIZE]; /**< name of the memory zone */
>
> phys_addr_t phys_addr; /**< start physical address */
> void *addr; /**< start virtual address */
> uint64_t len; /**< len of the memzone */
>
> int socket_id; /**< NUMA socket id */
> };
>
>
>Using Lists
>
>Using the minus character, it is possible to generate a bullet list. The
>minus signs must be column-aligned. If the minus sign is followed by a
>hash,
>then it generates an enumerated list. Refer to the official Doxygen
>documentation for more information.
> /**
> * A list of events:
> * - mouse events
> * -# mouse move event
> * -# mouse click event\n
> * More info about the click event.
> * -# mouse double click event
> * - keyboard events
> * -# key down event
> * -# key up event
> *
> * More text here.
> */
>
>
>See Also Sections
>
>The @see keyword can be used to highlight a link to an existing function,
>file, or URL. This directive should be placed on one line, without
>anything else, at the bottom of the documentation header.
> /**
> * (documentation of function, file, ...)
> *
> * @see rte_foo()
> * @see eal_memzone.c
> */
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 15:53 ` Wiles, Keith
@ 2015-04-08 16:16 ` Thomas Monjalon
2015-04-08 16:25 ` Wiles, Keith
` (2 more replies)
0 siblings, 3 replies; 62+ messages in thread
From: Thomas Monjalon @ 2015-04-08 16:16 UTC (permalink / raw)
To: Wiles, Keith, Butler, Siobhan A; +Cc: dev
2015-04-08 15:53, Wiles, Keith:
> One of the biggest problems with any style is helping the developer
> maintain the style. Using some tool does help and I have used astyle
> before, not bad code formatter. Here is a few that seem to be reasonable.
>
> http://astyle.sourceforge.net/
>
> http://uncrustify.sourceforge.net/
>
> http://sourceforge.net/projects/gcgreatcode/
I'm not sure it's a good idea to convert the codebase automatically.
The coding style must be a reference for new patches and they must be
automatically checked with a dedicated checkpatch tool.
By forbidding patches which don't comply, the codebase will be naturally
converted over time.
I didn't review this proposal yet.
My first comment is that it's too long to read :)
When a consensus is done, it must be added with a patch with custom
checkpatch addition.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 16:16 ` Thomas Monjalon
@ 2015-04-08 16:25 ` Wiles, Keith
2015-04-08 19:54 ` Butler, Siobhan A
2015-04-14 14:52 ` Bruce Richardson
2 siblings, 0 replies; 62+ messages in thread
From: Wiles, Keith @ 2015-04-08 16:25 UTC (permalink / raw)
To: Thomas Monjalon, Butler, Siobhan A; +Cc: dev
On 4/8/15, 11:16 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
>2015-04-08 15:53, Wiles, Keith:
>> One of the biggest problems with any style is helping the developer
>> maintain the style. Using some tool does help and I have used astyle
>> before, not bad code formatter. Here is a few that seem to be
>>reasonable.
>>
>> http://astyle.sourceforge.net/
>>
>> http://uncrustify.sourceforge.net/
>>
>> http://sourceforge.net/projects/gcgreatcode/
>
>I'm not sure it's a good idea to convert the codebase automatically.
>The coding style must be a reference for new patches and they must be
>automatically checked with a dedicated checkpatch tool.
I was not suggesting these tools be used automatically only to get code to
comply as it is being imported or patched. If we product a config file for
one of the above tools then everyone can review his code before sending
the patch.
>By forbidding patches which don't comply, the codebase will be naturally
>converted over time.
It is difficult to view patches to determine if they comply, which is the
only problem. Seeing the patched file is easier to see code format
problems IMO.
>
>I didn't review this proposal yet.
>My first comment is that it's too long to read :)
>When a consensus is done, it must be added with a patch with custom
>checkpatch addition.
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 16:16 ` Thomas Monjalon
2015-04-08 16:25 ` Wiles, Keith
@ 2015-04-08 19:54 ` Butler, Siobhan A
2015-04-14 14:21 ` Bruce Richardson
2015-04-14 14:52 ` Bruce Richardson
2 siblings, 1 reply; 62+ messages in thread
From: Butler, Siobhan A @ 2015-04-08 19:54 UTC (permalink / raw)
To: Thomas Monjalon, Wiles, Keith; +Cc: dev
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, April 8, 2015 5:16 PM
> To: Wiles, Keith; Butler, Siobhan A
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
>
> 2015-04-08 15:53, Wiles, Keith:
> > One of the biggest problems with any style is helping the developer
> > maintain the style. Using some tool does help and I have used astyle
> > before, not bad code formatter. Here is a few that seem to be reasonable.
> >
> > http://astyle.sourceforge.net/
> >
> > http://uncrustify.sourceforge.net/
> >
> > http://sourceforge.net/projects/gcgreatcode/
>
> I'm not sure it's a good idea to convert the codebase automatically.
> The coding style must be a reference for new patches and they must be
> automatically checked with a dedicated checkpatch tool.
> By forbidding patches which don't comply, the codebase will be naturally
> converted over time.
>
> I didn't review this proposal yet.
> My first comment is that it's too long to read :) When a consensus is done, it
> must be added with a patch with custom checkpatch addition.
Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the comments supplied so far and post a fresh version tomorrow.
Siobhan
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 19:54 ` Butler, Siobhan A
@ 2015-04-14 14:21 ` Bruce Richardson
2015-04-14 14:38 ` Neil Horman
0 siblings, 1 reply; 62+ messages in thread
From: Bruce Richardson @ 2015-04-14 14:21 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
On Wed, Apr 08, 2015 at 07:54:40PM +0000, Butler, Siobhan A wrote:
>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Wednesday, April 8, 2015 5:16 PM
> > To: Wiles, Keith; Butler, Siobhan A
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] tools brainstorming
> >
> > 2015-04-08 15:53, Wiles, Keith:
> > > One of the biggest problems with any style is helping the developer
> > > maintain the style. Using some tool does help and I have used astyle
> > > before, not bad code formatter. Here is a few that seem to be reasonable.
> > >
> > > http://astyle.sourceforge.net/
> > >
> > > http://uncrustify.sourceforge.net/
> > >
> > > http://sourceforge.net/projects/gcgreatcode/
> >
> > I'm not sure it's a good idea to convert the codebase automatically.
> > The coding style must be a reference for new patches and they must be
> > automatically checked with a dedicated checkpatch tool.
> > By forbidding patches which don't comply, the codebase will be naturally
> > converted over time.
> >
> > I didn't review this proposal yet.
> > My first comment is that it's too long to read :) When a consensus is done, it
> > must be added with a patch with custom checkpatch addition.
> Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the comments supplied so far and post a fresh version tomorrow.
> Siobhan
>
Just wondering here, are we looking to codify what the current predominant coding
style in DPDK *is* or what it *should be*?
There has been some good discussion on a variety of areas, but if we focus on
initially codifying what's there now, some issues become easier to resolve -
e.g. discussion of commenting style, since only C89 comments are allowed right now.
/Bruce
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 14:21 ` Bruce Richardson
@ 2015-04-14 14:38 ` Neil Horman
2015-04-14 14:47 ` Thomas Monjalon
0 siblings, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-14 14:38 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> On Wed, Apr 08, 2015 at 07:54:40PM +0000, Butler, Siobhan A wrote:
> >
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > To: Wiles, Keith; Butler, Siobhan A
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] tools brainstorming
> > >
> > > 2015-04-08 15:53, Wiles, Keith:
> > > > One of the biggest problems with any style is helping the developer
> > > > maintain the style. Using some tool does help and I have used astyle
> > > > before, not bad code formatter. Here is a few that seem to be reasonable.
> > > >
> > > > http://astyle.sourceforge.net/
> > > >
> > > > http://uncrustify.sourceforge.net/
> > > >
> > > > http://sourceforge.net/projects/gcgreatcode/
> > >
> > > I'm not sure it's a good idea to convert the codebase automatically.
> > > The coding style must be a reference for new patches and they must be
> > > automatically checked with a dedicated checkpatch tool.
> > > By forbidding patches which don't comply, the codebase will be naturally
> > > converted over time.
> > >
> > > I didn't review this proposal yet.
> > > My first comment is that it's too long to read :) When a consensus is done, it
> > > must be added with a patch with custom checkpatch addition.
> > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the comments supplied so far and post a fresh version tomorrow.
> > Siobhan
> >
>
> Just wondering here, are we looking to codify what the current predominant coding
> style in DPDK *is* or what it *should be*?
>
> There has been some good discussion on a variety of areas, but if we focus on
> initially codifying what's there now, some issues become easier to resolve -
> e.g. discussion of commenting style, since only C89 comments are allowed right now.
>
This is an excellent question. I think the answer is we should make the style
what we want it to be. That said, when there is a significant discrepancy behind
what is wanted and what is, we need to stop and ask ourselves why that exists,
and what our reasoning is for wanting the change.
Neil
> /Bruce
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 14:38 ` Neil Horman
@ 2015-04-14 14:47 ` Thomas Monjalon
2015-04-14 14:54 ` Bruce Richardson
0 siblings, 1 reply; 62+ messages in thread
From: Thomas Monjalon @ 2015-04-14 14:47 UTC (permalink / raw)
To: dev
2015-04-14 10:38, Neil Horman:
> On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> > On Wed, Apr 08, 2015 at 07:54:40PM +0000, Butler, Siobhan A wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > > To: Wiles, Keith; Butler, Siobhan A
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] tools brainstorming
> > > >
> > > > 2015-04-08 15:53, Wiles, Keith:
> > > > > One of the biggest problems with any style is helping the developer
> > > > > maintain the style. Using some tool does help and I have used astyle
> > > > > before, not bad code formatter. Here is a few that seem to be reasonable.
> > > > >
> > > > > http://astyle.sourceforge.net/
> > > > >
> > > > > http://uncrustify.sourceforge.net/
> > > > >
> > > > > http://sourceforge.net/projects/gcgreatcode/
> > > >
> > > > I'm not sure it's a good idea to convert the codebase automatically.
> > > > The coding style must be a reference for new patches and they must be
> > > > automatically checked with a dedicated checkpatch tool.
> > > > By forbidding patches which don't comply, the codebase will be naturally
> > > > converted over time.
> > > >
> > > > I didn't review this proposal yet.
> > > > My first comment is that it's too long to read :) When a consensus is done, it
> > > > must be added with a patch with custom checkpatch addition.
> > > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the comments supplied so far and post a fresh version tomorrow.
> > > Siobhan
> > >
> >
> > Just wondering here, are we looking to codify what the current predominant coding
> > style in DPDK *is* or what it *should be*?
> >
> > There has been some good discussion on a variety of areas, but if we focus on
> > initially codifying what's there now, some issues become easier to resolve -
> > e.g. discussion of commenting style, since only C89 comments are allowed right now.
> >
>
> This is an excellent question. I think the answer is we should make the style
> what we want it to be. That said, when there is a significant discrepancy behind
> what is wanted and what is, we need to stop and ask ourselves why that exists,
> and what our reasoning is for wanting the change.
Yes the question must be asked.
I think the main goal is to have a consistent style.
As there is already a lot of code with implicit guidelines,
it's simpler to make them official.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 14:47 ` Thomas Monjalon
@ 2015-04-14 14:54 ` Bruce Richardson
0 siblings, 0 replies; 62+ messages in thread
From: Bruce Richardson @ 2015-04-14 14:54 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
On Tue, Apr 14, 2015 at 04:47:47PM +0200, Thomas Monjalon wrote:
> 2015-04-14 10:38, Neil Horman:
> > On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> > > On Wed, Apr 08, 2015 at 07:54:40PM +0000, Butler, Siobhan A wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > > > To: Wiles, Keith; Butler, Siobhan A
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] tools brainstorming
> > > > >
> > > > > 2015-04-08 15:53, Wiles, Keith:
> > > > > > One of the biggest problems with any style is helping the developer
> > > > > > maintain the style. Using some tool does help and I have used astyle
> > > > > > before, not bad code formatter. Here is a few that seem to be reasonable.
> > > > > >
> > > > > > http://astyle.sourceforge.net/
> > > > > >
> > > > > > http://uncrustify.sourceforge.net/
> > > > > >
> > > > > > http://sourceforge.net/projects/gcgreatcode/
> > > > >
> > > > > I'm not sure it's a good idea to convert the codebase automatically.
> > > > > The coding style must be a reference for new patches and they must be
> > > > > automatically checked with a dedicated checkpatch tool.
> > > > > By forbidding patches which don't comply, the codebase will be naturally
> > > > > converted over time.
> > > > >
> > > > > I didn't review this proposal yet.
> > > > > My first comment is that it's too long to read :) When a consensus is done, it
> > > > > must be added with a patch with custom checkpatch addition.
> > > > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the comments supplied so far and post a fresh version tomorrow.
> > > > Siobhan
> > > >
> > >
> > > Just wondering here, are we looking to codify what the current predominant coding
> > > style in DPDK *is* or what it *should be*?
> > >
> > > There has been some good discussion on a variety of areas, but if we focus on
> > > initially codifying what's there now, some issues become easier to resolve -
> > > e.g. discussion of commenting style, since only C89 comments are allowed right now.
> > >
> >
> > This is an excellent question. I think the answer is we should make the style
> > what we want it to be. That said, when there is a significant discrepancy behind
> > what is wanted and what is, we need to stop and ask ourselves why that exists,
> > and what our reasoning is for wanting the change.
>
> Yes the question must be asked.
> I think the main goal is to have a consistent style.
> As there is already a lot of code with implicit guidelines,
> it's simpler to make them official.
>
Sounds good to me. Let's document what we have, then evolve it as necessary. :-)
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 16:16 ` Thomas Monjalon
2015-04-08 16:25 ` Wiles, Keith
2015-04-08 19:54 ` Butler, Siobhan A
@ 2015-04-14 14:52 ` Bruce Richardson
2015-04-14 15:24 ` Thomas Monjalon
2 siblings, 1 reply; 62+ messages in thread
From: Bruce Richardson @ 2015-04-14 14:52 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
> 2015-04-08 15:53, Wiles, Keith:
> > One of the biggest problems with any style is helping the developer
> > maintain the style. Using some tool does help and I have used astyle
> > before, not bad code formatter. Here is a few that seem to be reasonable.
> >
> > http://astyle.sourceforge.net/
> >
> > http://uncrustify.sourceforge.net/
> >
> > http://sourceforge.net/projects/gcgreatcode/
>
> I'm not sure it's a good idea to convert the codebase automatically.
> The coding style must be a reference for new patches and they must be
> automatically checked with a dedicated checkpatch tool.
> By forbidding patches which don't comply, the codebase will be naturally
> converted over time.
>
I'd like to see us document the existing style as much as possible before changing
it. That saves any conversion issues. In cases where multiple styles are used,
we can initially go with the more prevalent one.
> I didn't review this proposal yet.
> My first comment is that it's too long to read :)
> When a consensus is done, it must be added with a patch with custom
> checkpatch addition.
>
My personal feeling is that we should try and keep checkpatch modifications to a
minimum. Right now, we can use checkpatch as-is from kernel.org, right?
/Bruce
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 14:52 ` Bruce Richardson
@ 2015-04-14 15:24 ` Thomas Monjalon
2015-04-14 16:19 ` Wiles, Keith
0 siblings, 1 reply; 62+ messages in thread
From: Thomas Monjalon @ 2015-04-14 15:24 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
2015-04-14 15:52, Bruce Richardson:
> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
> > When a consensus is done, it must be added with a patch with custom
> > checkpatch addition.
> >
> My personal feeling is that we should try and keep checkpatch modifications to a
> minimum. Right now, we can use checkpatch as-is from kernel.org, right?
Yes that's something we have to discuss.
It should be preferred to avoid "forking" checkpatch.
At the moment, I'm using this configuration:
options="$options --max-line-length=100"
options="$options --show-types"
options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"
linux/scripts/checkpatch.pl $options
I would like to submit a script to run checkpatch with DPDK configuration
when the coding rules are clear.
However, I've already seen some options which are not enough configurable
(don't remember which one). For such corner case, I would see 3 solutions
(from the most to the least desired):
- submit a patch to allow more configuration to kernel.org
- give up automatic handling of corner cases
- maintain a fork in scripts/ directory
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 15:24 ` Thomas Monjalon
@ 2015-04-14 16:19 ` Wiles, Keith
2015-04-14 18:52 ` Wiles, Keith
0 siblings, 1 reply; 62+ messages in thread
From: Wiles, Keith @ 2015-04-14 16:19 UTC (permalink / raw)
To: Thomas Monjalon, Richardson, Bruce; +Cc: dev
On 4/14/15, 10:24 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
>2015-04-14 15:52, Bruce Richardson:
>> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
>> > When a consensus is done, it must be added with a patch with custom
>> > checkpatch addition.
>> >
>> My personal feeling is that we should try and keep checkpatch
>>modifications to a
>> minimum. Right now, we can use checkpatch as-is from kernel.org, right?
>
>Yes that's something we have to discuss.
>It should be preferred to avoid "forking" checkpatch.
>
>At the moment, I'm using this configuration:
>
> options="$options --max-line-length=100"
> options="$options --show-types"
> options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
> SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"
>
> linux/scripts/checkpatch.pl $options
>
>I would like to submit a script to run checkpatch with DPDK configuration
>when the coding rules are clear.
>
>However, I've already seen some options which are not enough configurable
>(don't remember which one). For such corner case, I would see 3 solutions
>(from the most to the least desired):
> - submit a patch to allow more configuration to kernel.org
> - give up automatic handling of corner cases
> - maintain a fork in scripts/ directory
Here is the next solution
- Stop using checkpatch and use a real tool for formatting code instead.
If someone uses a tool before commit, then create the patch which does not
require checkpatch.
Most of these tools can define an output file or they leave behind the
original file as a backup or we can see if they have a non-modify mode and
just points out the problems. As in astyle '--dry-run' can be used, plus
it saves the original file as XXXXX.orig or you can change the .orig to
your own value.
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-14 16:19 ` Wiles, Keith
@ 2015-04-14 18:52 ` Wiles, Keith
0 siblings, 0 replies; 62+ messages in thread
From: Wiles, Keith @ 2015-04-14 18:52 UTC (permalink / raw)
To: Wiles, Keith, Thomas Monjalon, Richardson, Bruce; +Cc: dev
On 4/14/15, 11:19 AM, "Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>
>On 4/14/15, 10:24 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
>
>>2015-04-14 15:52, Bruce Richardson:
>>> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
>>> > When a consensus is done, it must be added with a patch with custom
>>> > checkpatch addition.
>>> >
>>> My personal feeling is that we should try and keep checkpatch
>>>modifications to a
>>> minimum. Right now, we can use checkpatch as-is from kernel.org, right?
>>
>>Yes that's something we have to discuss.
>>It should be preferred to avoid "forking" checkpatch.
>>
>>At the moment, I'm using this configuration:
>>
>> options="$options --max-line-length=100"
>> options="$options --show-types"
>> options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
>> VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
>> SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"
>>
>> linux/scripts/checkpatch.pl $options
>>
>>I would like to submit a script to run checkpatch with DPDK configuration
>>when the coding rules are clear.
>>
>>However, I've already seen some options which are not enough configurable
>>(don't remember which one). For such corner case, I would see 3 solutions
>>(from the most to the least desired):
>> - submit a patch to allow more configuration to kernel.org
>> - give up automatic handling of corner cases
>> - maintain a fork in scripts/ directory
>Here is the next solution
> - Stop using checkpatch and use a real tool for formatting code instead.
>If someone uses a tool before commit, then create the patch which does not
>require checkpatch.
>Most of these tools can define an output file or they leave behind the
>original file as a backup or we can see if they have a non-modify mode and
>just points out the problems. As in astyle '--dry-run' can be used, plus
>it saves the original file as XXXXX.orig or you can change the .orig to
>your own value.
>>
Using uncrustify with following config file seems to be very close to what
we have today and removes trailing white spaces. I changed the
indent_with_tabs to 2 instead of 1. I am sure we could even get closer.
The uncrustify creates the updated file into XXXXX.uncrustify which does
not effect the original file. Then you can use meld or some other tool to
view the changes.
http://uncrustify.sourceforge.net/default.cfg
The source is here http://uncrustify.sourceforge.net/ pretty simple
install on my Ubuntu 14.04 machine.
I also installed the amd64 image of UniversalindentGUI. I tried the
ŒUbuntu Software Center¹ version, but it required a bit more effort then
just installing. It appeared the code needed a specific version of a
library I did not track down.
http://uncrustify.sourceforge.net/
It looks like using uncrustify and a config file gives use a tool to
verify the code is formatted correctly before commit and patch create.
Have a look and see what you think.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
` (3 preceding siblings ...)
2015-04-08 15:53 ` Wiles, Keith
@ 2015-04-08 18:16 ` Stephen Hemminger
2015-04-08 18:58 ` Matthew Hall
` (2 more replies)
2015-04-08 21:55 ` Don Provan
` (2 subsequent siblings)
7 siblings, 3 replies; 62+ messages in thread
From: Stephen Hemminger @ 2015-04-08 18:16 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
Thanks for doing this, it is a great start.
I admit strong bias towards Linux kernel style.
Could you use one of the standard markup styles so that it could get put in documentation?
> License Header
> --------------
I prefer the file just say that it is BSD or GPL and refer to license files in the
package. That way if something has to change it doesn't need a massive license sweep
>
> Macros
>
> Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces.
>
> The names of ``unsafe`` macros (ones that have side effects), and the names of macros for manifest constants, are all in uppercase.
>
> The expansions of expression-like macros are either a single token or have outer parentheses. If a macro is an inline expansion of a function,
> the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes;
> it makes it easier to read. If the macro encapsulates a compound statement, enclose it in a do loop, so that it can be used safely in if statements.
> Any final statement-terminating semicolon should be supplied by the macro invocation rather than the macro, to make parsing easier for pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
^ bad whitespace
it is important that all examples in documentation are perfect.
> C Function Definition, Declaration and Use
>
> Prototypes
>
> It is recommended, but not required that all functions are prototyped somewhere.
>
> Any function prototypes for private functions (that is, functions not used elsewhere) go at the top of the first source module. Functions
> local to one source module should be declared static.
I find prototypes for private functions to be redundant and error prone.
The do nothing. Better to just put private functions in the correct order.
You also need to raise the issue that all global names need to be prefaced by a unique string.
I see places in drivers where global names leak out causing possible later name collision.
> Definitions
> -----------
>
> The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
Not a big fan of that style. Prefer it on same line.
>
> Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
Also no spaces before tabs.
> NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> If you have to wrap a long statement, put the operator at the end of the line, and indent again. For control statements (if, while, etc.),
> it is recommended that the next line be indented by two tabs, rather than one, to prevent confusion as to whether the second line of the
> control statement forms part of the statement body or not. For non-control statements, this issue does not apply, so they can be indented
> by a single tab. However, a two-tab indent is recommended in this case also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
No. Should line up with really_long_variable_name_1
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
> Do not add whitespace at the end of a line.
>
> Closing and opening braces go on the same line as the else keyword. Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
> Function Calls
> --------------
>
> Do not use spaces after function names. Commas should have a space after them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
>
>
> Operators
> ---------
>
> Unary operators do not require spaces, binary operators do. Do not use parentheses unless they are required for precedence or unless the
> statement is confusing without them. Remember that other people may be more easily confused than you.
>
> Exit
>
> Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
+11
>
> Return Value
> ------------
>
> If possible, functions should return 0 on success and a negative value on error. The negative value should be ``-errno`` if relevant, for example, ``-EINVAL``.
>
> Routines returning ``void *`` should not have their return values cast to any pointer type.
> (Typecasting can prevent the compiler from warning about missing prototypes as any implicit definition of a function returns int - which, unlike "void *" needs a typecast to assign to a pointer variable.)
> NOTE The above rule applies to malloc, as well as to DPDK functions.
> Values in return statements should be enclosed in parentheses.
Sorry, this is a stupid BSDism
return (-EINVAL);
is ugly
>
> Logging and Errors
> ------------------
>
> In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
>
> In a userland program that is not a DPDK application, use err(3) or warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
^ NO BSD style return ()
>
> Branch Prediction
> -----------------
>
> When a test is done in a critical zone (called often or in a data path) use the ``likely()`` and ``unlikely()`` macros. They are expanded
> as a compiler builtin and allow the developer to indicate if the branch is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
You need to stress that likely() and unlikely() should be used sparingly.
Many times the compiler knows better.
>
>
> Static Variables and Functions
> ------------------------------
>
> All functions and variables that are local to a file must be declared as ``static`` because it can often help the compiler to do
> some optimizations (such as, inlining the code).
>
> Functions that must be inlined have to be declared as ``static inline`` and can be defined in a .c or a .h file.
>
> Const Attribute
> ---------------
>
> Particular care must be taken with the use of the ``const`` attribute. It should be used as often as possible when a variable is read-only.
A couple more things:
no UPPER or CamelCase variable names or functions
The other thing (and Intel is the worst offender) is to avoid excessive inlining.
For example, I am pretty sure that all rte_ring stuff really should not be inline,
the compiler can do a good job (especially with LTO) even without inlining.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 18:16 ` Stephen Hemminger
@ 2015-04-08 18:58 ` Matthew Hall
2015-04-08 22:12 ` Stephen Hemminger
2015-04-08 19:51 ` Butler, Siobhan A
2015-04-14 15:29 ` Bruce Richardson
2 siblings, 1 reply; 62+ messages in thread
From: Matthew Hall @ 2015-04-08 18:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Wed, Apr 08, 2015 at 11:16:03AM -0700, Stephen Hemminger wrote:
> I prefer the file just say that it is BSD or GPL and refer to license files
> in the package. That way if something has to change it doesn't need a
> massive license sweep
Hi guys,
I hope we're also enforcing some requirement that all user-space files that
are expected to be used inside of the address space apps must be BSD, MIT, or
other license which allows binary redistribution, as part of these standards.
Or we could end up causing a lot of pain for the app developers if somebody
puts a bunch of GPL files into the user-space code which blocks their usage.
For the Linux kernel side files, we probably need to say BSD, MIT, or GPLv2
specifically, and not GPLv3, I think that's what Linus is using, or it could
be a problem to upstream any of those as DPDK usage grows.
For the BSD kernel side files, if any, probably need to be sure we're
compatible with at least FreeBSD and NetBSD, and probably also OpenBSD.
Matthew.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 18:58 ` Matthew Hall
@ 2015-04-08 22:12 ` Stephen Hemminger
0 siblings, 0 replies; 62+ messages in thread
From: Stephen Hemminger @ 2015-04-08 22:12 UTC (permalink / raw)
To: Matthew Hall; +Cc: dev
The userspace code has to be BSD ( no GPL ).
The Linux kernel code must be GPLv2 or BSD/GPL license.
The other kernel code should be BSD/GPL.
On Wed, Apr 8, 2015 at 11:58 AM, Matthew Hall <mhall@mhcomputing.net> wrote:
> On Wed, Apr 08, 2015 at 11:16:03AM -0700, Stephen Hemminger wrote:
> > I prefer the file just say that it is BSD or GPL and refer to license
> files
> > in the package. That way if something has to change it doesn't need a
> > massive license sweep
>
> Hi guys,
>
> I hope we're also enforcing some requirement that all user-space files that
> are expected to be used inside of the address space apps must be BSD, MIT,
> or
> other license which allows binary redistribution, as part of these
> standards.
> Or we could end up causing a lot of pain for the app developers if somebody
> puts a bunch of GPL files into the user-space code which blocks their
> usage.
>
> For the Linux kernel side files, we probably need to say BSD, MIT, or GPLv2
> specifically, and not GPLv3, I think that's what Linus is using, or it
> could
> be a problem to upstream any of those as DPDK usage grows.
>
> For the BSD kernel side files, if any, probably need to be sure we're
> compatible with at least FreeBSD and NetBSD, and probably also OpenBSD.
>
> Matthew.
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 18:16 ` Stephen Hemminger
2015-04-08 18:58 ` Matthew Hall
@ 2015-04-08 19:51 ` Butler, Siobhan A
2015-04-14 15:29 ` Bruce Richardson
2 siblings, 0 replies; 62+ messages in thread
From: Butler, Siobhan A @ 2015-04-08 19:51 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, April 8, 2015 7:16 PM
> To: Butler, Siobhan A
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
>
> Thanks for doing this, it is a great start.
> I admit strong bias towards Linux kernel style.
>
> Could you use one of the standard markup styles so that it could get put in
> documentation?
Sure Stephen - will tidy it all up when we get a consensus :)
Thanks for the feedback
Siobhan
>
>
> > License Header
> > --------------
>
> I prefer the file just say that it is BSD or GPL and refer to license files in the
> package. That way if something has to change it doesn't need a massive
> license sweep
>
>
> >
> > Macros
> >
> > Do not ``#define`` or declare names in the implementation namespace
> except for implementing application interfaces.
> >
> > The names of ``unsafe`` macros (ones that have side effects), and the
> names of macros for manifest constants, are all in uppercase.
> >
> > The expansions of expression-like macros are either a single token or
> > have outer parentheses. If a macro is an inline expansion of a
> > function, the function name is all in lowercase and the macro has the same
> name all in uppercase. Right-justify the backslashes; it makes it easier to
> read. If the macro encapsulates a compound statement, enclose it in a do
> loop, so that it can be used safely in if statements.
> > Any final statement-terminating semicolon should be supplied by the
> macro invocation rather than the macro, to make parsing easier for pretty-
> printers and editors.
> > #define MACRO(x, y) do { \
> > variable = (x) + (y); \
> > (y) += 2; \
> > }while (0)
> ^ bad whitespace
>
> it is important that all examples in documentation are perfect.
>
>
> > C Function Definition, Declaration and Use
> >
> > Prototypes
> >
> > It is recommended, but not required that all functions are prototyped
> somewhere.
> >
> > Any function prototypes for private functions (that is, functions not used
> elsewhere) go at the top of the first source module. Functions
> > local to one source module should be declared static.
>
> I find prototypes for private functions to be redundant and error prone.
> The do nothing. Better to just put private functions in the correct order.
>
>
> You also need to raise the issue that all global names need to be prefaced by
> a unique string.
> I see places in drivers where global names leak out causing possible later
> name collision.
>
> > Definitions
> > -----------
> >
> > The function type should be on a line by itself preceding the function. The
> opening brace of the function body should be on a line by itself.
> > static char *
> > function(int a1, int a2, float fl, int a4)
> > {
>
> Not a big fan of that style. Prefer it on same line.
>
>
> >
> > Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
>
> Also no spaces before tabs.
>
> > NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> > If you have to wrap a long statement, put the operator at the end of the
> line, and indent again. For control statements (if, while, etc.),
> > it is recommended that the next line be indented by two tabs, rather than
> one, to prevent confusion as to whether the second line of the
> > control statement forms part of the statement body or not. For non-
> control statements, this issue does not apply, so they can be indented
> > by a single tab. However, a two-tab indent is recommended in this case
> also to keep consistency across all statement types.
> > while (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){
> > x = y + z; /* control stmt body lines up with second line of */
> > a = b + c; /* control statement itself if single indent used */
> > }
> >
> > if (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){ /* two tabs used */
>
> No. Should line up with really_long_variable_name_1
>
> > x = y + z; /* statement body no longer lines up */
> > a = b + c;
> > }
> >
> > z = a + really + long + statement + that + needs +
> > two + lines + gets + indented + on + the +
> > second + and + subsequent + lines;
> >
> >
> > Do not add whitespace at the end of a line.
> >
> > Closing and opening braces go on the same line as the else keyword. Braces
> that are not necessary should be left out.
> > if (test)
> > stmt;
> > else if (bar) {
> > stmt;
> > stmt;
> > } else
> > stmt;
> >
> >
> > Function Calls
> > --------------
> >
> > Do not use spaces after function names. Commas should have a space after
> them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> > error = function(a1, a2);
> > if (error != 0)
> > exit(error);
> >
> >
> > Operators
> > ---------
> >
> > Unary operators do not require spaces, binary operators do. Do not use
> parentheses unless they are required for precedence or unless the
> > statement is confusing without them. Remember that other people may
> be more easily confused than you.
> >
> > Exit
> >
> > Exits should be 0 on success, or 1 on failure.
> > exit(0); /*
> > * Avoid obvious comments such as
> > * "Exit 0 on success."
> > */
>
> +11
>
>
> >
> > Return Value
> > ------------
> >
> > If possible, functions should return 0 on success and a negative value on
> error. The negative value should be ``-errno`` if relevant, for example, ``-
> EINVAL``.
> >
> > Routines returning ``void *`` should not have their return values cast to any
> pointer type.
> > (Typecasting can prevent the compiler from warning about missing
> prototypes as any implicit definition of a function returns int - which, unlike
> "void *" needs a typecast to assign to a pointer variable.)
> > NOTE The above rule applies to malloc, as well as to DPDK functions.
> > Values in return statements should be enclosed in parentheses.
>
> Sorry, this is a stupid BSDism
> return (-EINVAL);
> is ugly
>
> >
> > Logging and Errors
> > ------------------
> >
> > In the DPDK environment, use the logging interface provided::
> > #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> > #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
> >
> > /* enable these logs type */
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
> >
> > /* log in debug level */
> > rte_set_log_level(RTE_LOG_DEBUG);
> > RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> > RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> > RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
> >
> > /* log in info level */
> > rte_set_log_level(RTE_LOG_INFO);
> > RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
> >
> >
> > In a userland program that is not a DPDK application, use err(3) or warn(3).
> Do not create your own variant.
> > if ((four = malloc(sizeof(struct foo))) == NULL)
> > err(1, (char *)NULL);
> > if ((six = (int *)overflow()) == NULL)
> > errx(1, "number overflowed");
> > return (eight);
> ^ NO BSD style return ()
>
> >
> > Branch Prediction
> > -----------------
> >
> > When a test is done in a critical zone (called often or in a data path) use the
> ``likely()`` and ``unlikely()`` macros. They are expanded
> > as a compiler builtin and allow the developer to indicate if the branch is
> likely to be taken or not. Example:
> > #include <rte_branch_prediction.h>
> > if (likely(x > 1))
> > do_stuff();
>
> You need to stress that likely() and unlikely() should be used sparingly.
> Many times the compiler knows better.
>
> >
> >
> > Static Variables and Functions
> > ------------------------------
> >
> > All functions and variables that are local to a file must be declared as
> ``static`` because it can often help the compiler to do
> > some optimizations (such as, inlining the code).
> >
> > Functions that must be inlined have to be declared as ``static inline`` and
> can be defined in a .c or a .h file.
> >
> > Const Attribute
> > ---------------
> >
> > Particular care must be taken with the use of the ``const`` attribute. It
> should be used as often as possible when a variable is read-only.
>
>
> A couple more things:
> no UPPER or CamelCase variable names or functions
>
> The other thing (and Intel is the worst offender) is to avoid excessive inlining.
> For example, I am pretty sure that all rte_ring stuff really should not be
> inline,
> the compiler can do a good job (especially with LTO) even without inlining.
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 18:16 ` Stephen Hemminger
2015-04-08 18:58 ` Matthew Hall
2015-04-08 19:51 ` Butler, Siobhan A
@ 2015-04-14 15:29 ` Bruce Richardson
2 siblings, 0 replies; 62+ messages in thread
From: Bruce Richardson @ 2015-04-14 15:29 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
On Wed, Apr 08, 2015 at 11:16:03AM -0700, Stephen Hemminger wrote:
> Thanks for doing this, it is a great start.
> I admit strong bias towards Linux kernel style.
>
> Could you use one of the standard markup styles so that it could get put in documentation?
>
>
> > License Header
> > --------------
>
> I prefer the file just say that it is BSD or GPL and refer to license files in the
> package. That way if something has to change it doesn't need a massive license sweep
>
>
> >
> > Macros
> >
> > Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces.
> >
> > The names of ``unsafe`` macros (ones that have side effects), and the names of macros for manifest constants, are all in uppercase.
> >
> > The expansions of expression-like macros are either a single token or have outer parentheses. If a macro is an inline expansion of a function,
> > the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes;
> > it makes it easier to read. If the macro encapsulates a compound statement, enclose it in a do loop, so that it can be used safely in if statements.
> > Any final statement-terminating semicolon should be supplied by the macro invocation rather than the macro, to make parsing easier for pretty-printers and editors.
> > #define MACRO(x, y) do { \
> > variable = (x) + (y); \
> > (y) += 2; \
> > }while (0)
> ^ bad whitespace
>
> it is important that all examples in documentation are perfect.
>
>
> > C Function Definition, Declaration and Use
> >
> > Prototypes
> >
> > It is recommended, but not required that all functions are prototyped somewhere.
> >
> > Any function prototypes for private functions (that is, functions not used elsewhere) go at the top of the first source module. Functions
> > local to one source module should be declared static.
>
> I find prototypes for private functions to be redundant and error prone.
> The do nothing. Better to just put private functions in the correct order.
>
>
> You also need to raise the issue that all global names need to be prefaced by a unique string.
> I see places in drivers where global names leak out causing possible later name collision.
>
+1 to both.
> > Definitions
> > -----------
> >
> > The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself.
> > static char *
> > function(int a1, int a2, float fl, int a4)
> > {
>
> Not a big fan of that style. Prefer it on same line.
>
>
> >
> > Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
>
> Also no spaces before tabs.
>
> > NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> > If you have to wrap a long statement, put the operator at the end of the line, and indent again. For control statements (if, while, etc.),
> > it is recommended that the next line be indented by two tabs, rather than one, to prevent confusion as to whether the second line of the
> > control statement forms part of the statement body or not. For non-control statements, this issue does not apply, so they can be indented
> > by a single tab. However, a two-tab indent is recommended in this case also to keep consistency across all statement types.
> > while (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){
> > x = y + z; /* control stmt body lines up with second line of */
> > a = b + c; /* control statement itself if single indent used */
> > }
> >
> > if (really_long_variable_name_1 == really_long_variable_name_2 &&
> > var3 == var4){ /* two tabs used */
>
> No. Should line up with really_long_variable_name_1
>
I disagree with that. For a couple of reasons:
*. It means using spaces as well as tabs for indentation, while I think either
one or the other should be used, not both.
*. For anyone using a 4-character tab-stop display, the var3 line will line up
visually with the body of the block making it look like the body, rather than
part of the condition. For anyone using an 8-character tab, the same effect
will be got with a while statement with a couple of opening braces. By using
two tabs, we guarantee that the line continuation never lines up with the
body of the block.
> > x = y + z; /* statement body no longer lines up */
> > a = b + c;
> > }
> >
> > z = a + really + long + statement + that + needs +
> > two + lines + gets + indented + on + the +
> > second + and + subsequent + lines;
> >
> >
> > Do not add whitespace at the end of a line.
> >
> > Closing and opening braces go on the same line as the else keyword. Braces that are not necessary should be left out.
> > if (test)
> > stmt;
> > else if (bar) {
> > stmt;
> > stmt;
> > } else
> > stmt;
> >
> >
> > Function Calls
> > --------------
> >
> > Do not use spaces after function names. Commas should have a space after them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> > error = function(a1, a2);
> > if (error != 0)
> > exit(error);
> >
> >
> > Operators
> > ---------
> >
> > Unary operators do not require spaces, binary operators do. Do not use parentheses unless they are required for precedence or unless the
> > statement is confusing without them. Remember that other people may be more easily confused than you.
> >
> > Exit
> >
> > Exits should be 0 on success, or 1 on failure.
> > exit(0); /*
> > * Avoid obvious comments such as
> > * "Exit 0 on success."
> > */
>
> +11
>
>
> >
> > Return Value
> > ------------
> >
> > If possible, functions should return 0 on success and a negative value on error. The negative value should be ``-errno`` if relevant, for example, ``-EINVAL``.
> >
> > Routines returning ``void *`` should not have their return values cast to any pointer type.
> > (Typecasting can prevent the compiler from warning about missing prototypes as any implicit definition of a function returns int - which, unlike "void *" needs a typecast to assign to a pointer variable.)
> > NOTE The above rule applies to malloc, as well as to DPDK functions.
> > Values in return statements should be enclosed in parentheses.
>
> Sorry, this is a stupid BSDism
> return (-EINVAL);
> is ugly
>
Agree.
> >
> > Logging and Errors
> > ------------------
> >
> > In the DPDK environment, use the logging interface provided::
> > #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> > #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
> >
> > /* enable these logs type */
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> > rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
> >
> > /* log in debug level */
> > rte_set_log_level(RTE_LOG_DEBUG);
> > RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> > RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> > RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
> >
> > /* log in info level */
> > rte_set_log_level(RTE_LOG_INFO);
> > RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
> >
> >
> > In a userland program that is not a DPDK application, use err(3) or warn(3). Do not create your own variant.
> > if ((four = malloc(sizeof(struct foo))) == NULL)
> > err(1, (char *)NULL);
> > if ((six = (int *)overflow()) == NULL)
> > errx(1, "number overflowed");
> > return (eight);
> ^ NO BSD style return ()
>
> >
> > Branch Prediction
> > -----------------
> >
> > When a test is done in a critical zone (called often or in a data path) use the ``likely()`` and ``unlikely()`` macros. They are expanded
> > as a compiler builtin and allow the developer to indicate if the branch is likely to be taken or not. Example:
> > #include <rte_branch_prediction.h>
> > if (likely(x > 1))
> > do_stuff();
>
> You need to stress that likely() and unlikely() should be used sparingly.
> Many times the compiler knows better.
>
> >
> >
> > Static Variables and Functions
> > ------------------------------
> >
> > All functions and variables that are local to a file must be declared as ``static`` because it can often help the compiler to do
> > some optimizations (such as, inlining the code).
> >
> > Functions that must be inlined have to be declared as ``static inline`` and can be defined in a .c or a .h file.
> >
> > Const Attribute
> > ---------------
> >
> > Particular care must be taken with the use of the ``const`` attribute. It should be used as often as possible when a variable is read-only.
>
>
> A couple more things:
> no UPPER or CamelCase variable names or functions
>
> The other thing (and Intel is the worst offender) is to avoid excessive inlining.
> For example, I am pretty sure that all rte_ring stuff really should not be inline,
> the compiler can do a good job (especially with LTO) even without inlining.
>
Disagree here too. When you are writing code while is polling from a software
ring which is likely to be empty a large proportion of the time, e.g. a control
queue, the cycle cost of the function call overhead is far greater than the time
it takes the poll the empty ring itself. [Measured using the ring unit tests]
/Bruce
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
` (4 preceding siblings ...)
2015-04-08 18:16 ` Stephen Hemminger
@ 2015-04-08 21:55 ` Don Provan
2015-04-13 15:02 ` Neil Horman
2015-04-16 10:49 ` Thomas Monjalon
7 siblings, 0 replies; 62+ messages in thread
From: Don Provan @ 2015-04-08 21:55 UTC (permalink / raw)
To: Butler, Siobhan A, Thomas Monjalon, dev
>NOTE Please avoid, as much as possible, including headers from other headers file. Doing so should be properly explained and justified.
Actually, I think a *failure* to #include other header files that this header file depends on should be what needs explained and justified. It drives me crazy when a header file forces me to figure out what header files it depends on and then forces me to include them in my sources even though my sources don't use them. Especially when #include is such a straightforward way to document the dependency while keeping me out of it.
Or are you only talking about when the header file doesn't depend on the header files its #including? If so, then I'd prefer ruling it out entirely rather than saying it needs to be justified.
>For consistency, getopt(3) should be used to parse options.
I assume this means the getopt() family, and I'd expect getopt_long() to be used normally. (If it were me, I'd *encourage* getopt_long() over getopt().)
>not::
> if (!*p)
I'm not sure why you'd bother to rule out this common idiom or the similar NULL pointer check. Are "if (*p)" and "if (p)" also prohibited, or just their negations?
>Values in return statements should be enclosed in parentheses.
Please don't encourage people to have this silly habit. It makes no more sense than insisting variables be set with "x = (-1)".
>static void
> usage()
This has nothing to do with the point being made here in your document, but surely you want to insist on "static void usage(void)", right? In fact, you might mention parameterless functions explicitly in the section on function declarations.
Everything else looks pretty cool. I'm surprised and impressed.
-don provan
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
` (5 preceding siblings ...)
2015-04-08 21:55 ` Don Provan
@ 2015-04-13 15:02 ` Neil Horman
2015-04-13 23:44 ` Stephen Hemminger
2015-04-16 10:49 ` Thomas Monjalon
7 siblings, 1 reply; 62+ messages in thread
From: Neil Horman @ 2015-04-13 15:02 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
On Wed, Apr 08, 2015 at 10:43:53AM +0000, Butler, Siobhan A wrote:
> Hi all,
> To add to the tools brainstorming - I propose we use the following Coding Standards as the basis of guidelines on coding style going forward.
> The style outlined below is in alignment with the current convention used for the majority of the project.
> Any thoughts/suggestions or feedback welcome.
> Thanks
> Siobhan :)
> <siobhan.a.butler@intel.com>
>
>
>
> Coding Style
> ~~~~~~~~~~
>
> Description
> -----------
>
> This document specifies the preferred style for source files in the DPDK source tree.
> It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2 Kernel Developer's Manual (see man style(9)),
> but was heavily modified for the needs of the DPDK. Many of the style rules are implicit in the examples.
> Be careful to check the examples before assuming that style is silent on an issue.
>
> General Guidelines
> ------------------
>
> The rules and guidelines given in this document cannot cover every situation, so the following general guidelines should be used as a fallback:
> The code style should be consistent within each individual file, and within each file in a given directory or module - in the case of creating new files
> The primary reason for coding standards is to increase code readability and comprehensibility, therefore always use whatever option will make the code easiest to read.
>
> The following more specific recommendations apply to all sections, both for C and assembly code:
> Line length is recommended to be not more than 80 characters, including comments. [Tab stop size should be assumed to be at least 4-characters wide]
> Indentation should be to no more than 3 levels deep.
> NOTE The above are recommendations, and not hard limits. However, it is expected that the recommendations should be followed in all but the rarest situations.
> C Comment Style
>
> Usual Comments
> --------------
>
> These comments should be used in normal cases. To document a public API, a doxygen-like format must be used: refer to Doxygen Documentation.
> /*
> * VERY important single-line comments look like this.
> */
>
> /* Most single-line comments look like this. */
>
> /*
> * Multi-line comments look like this. Make them real sentences. Fill
> * them so they look like real paragraphs.
> */
>
> License Header
> --------------
>
> Each file should begin with a special comment tag which will contain the appropriate copyright and license for the file (Generally BSD License).
> After any copyright header, a blank line should be left before any other contents, e.g. include statements in a C file.
>
> C Preprocessor Directives
> -------------------------
>
> Header Includes
>
> In DPDK sources, the include files should be ordered as following:
> libc includes (system includes first)
> DPDK EAL includes
> DPDK misc libraries includes
> application-specific includes
>
> Example:
> #include <stdio.h>
> #include <stdlib.h>
>
> #include <rte_eal.h>
>
> #include <rte_ring.h>
> #include <rte_mempool.h>
>
> #include "application.h"
It doesn't really matter to me, for the sake of consistency, it might be
worthwhile mandating search path includes only (< >), and adding a -I . to the
CFLAGS in the Makefile. That way a grep for "*.*<.*>" returns all your include
files
>
>
> Global pathnames are defined in <paths.h>. Pathnames local to the program go in "pathnames.h" in the local directory.
> #include <paths.h>
This is a design issue, not a coding style issue. Where an application chooses
to put its pathnames are its business, and not something we should codify here.
Also, paths.h doesn't exist so it should probably not be referenced here
>
>
> Leave another blank line before the user include files.
> #include "pathnames.h" /* Local includes in double quotes. */
>
> NOTE Please avoid, as much as possible, including headers from other headers file. Doing so should be properly explained and justified.
> Headers should be protected against multiple inclusion with the usual:
Are you sure you want to do that?
[nhorman@hmsreliant dpdk]$ find . -name '*.h' | xargs grep include | wc -l
1300
What would the justification be? Its common practice to do this, so I'm not
sure why you would discourage it.
> #ifndef _FILE_H_
> #define _FILE_H_
>
> /* Code */
>
> #endif /* _FILE_H_ */
>
>
> Macros
>
> Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces.
>
I'm not sure I understand what this means. Can you clarify the intent here?
> The names of ``unsafe`` macros (ones that have side effects), and the names of macros for manifest constants, are all in uppercase.
>
> The expansions of expression-like macros are either a single token or have outer parentheses. If a macro is an inline expansion of a function,
> the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes;
> it makes it easier to read. If the macro encapsulates a compound statement, enclose it in a do loop, so that it can be used safely in if statements.
> Any final statement-terminating semicolon should be supplied by the macro invocation rather than the macro, to make parsing easier for pretty-printers and editors.
> #define MACRO(x, y) do { \
> variable = (x) + (y); \
> (y) += 2; \
> }while (0)
>
> NOTE Wherever possible, enums and typedefs should be preferred to macros, since they provide additional degrees
> of type-safety and can allow compilers to emit extra warnings about unsafe code.
>
> Conditional Compilation
> -----------------------
>
> When code is conditionally compiled using #ifdef or #if, a comment may be added following the matching #endif or #else to
> permit the reader to easily discern where conditionally compiled code regions end. This comment should be used only for
> (subjectively) long regions, regions greater than 20 lines, or where a series of nested #ifdef 's may be confusing to the reader.
> Exceptions may be made for cases where code is conditionally not compiled for the purposes of lint(1), even though the uncompiled
> region may be small. The comment should be separated from the #endif or #else by a single space. For short conditionally compiled regions,
> a closing comment should not be used.
>
> The comment for #endif should match the expression used in the corresponding #if or #ifdef. The comment for #else and #elif
> should match the inverse of the expression(s) used in the preceding #if and/or #elif statements. In the comments,
> the subexpression defined(FOO) is abbreviated as FOO. For the purposes of comments, #ifndef FOO is treated as #if !defined(FOO).
> #ifdef KTRACE
> #include <sys/ktrace.h>
> #endif
>
> #ifdef COMPAT_43
> /* A large region here, or other conditional code. */
> #else /* !COMPAT_43 */
> /* Or here. */
> #endif /* COMPAT_43 */
>
> #ifndef COMPAT_43
> /* Yet another large region here, or other conditional code. */
> #else /* COMPAT_43 */
> /* Or here. */
> #endif /* !COMPAT_43 */
>
> NOTE Conditional compilation should be used only when absolutely necessary, as it increases the number of target binaries that need to be built and tested.
> C Types
>
> Integers
>
> For fixed/minimum-size integer values, the project uses the form uintXX_t (from stdint.h) instead of older BSD-style integer identifiers of the form u_intXX_t.
>
> Enumerations
> ------------
>
> Enumeration values are all uppercase.
> enum enumtype { ONE, TWO } et;
>
>
> Bitfields
> ---------
>
> The developer should group bitfields that are included in the same integer, as follows:
> struct grehdr {
> uint16_t rec:3,
> srr:1,
> seq:1,
> key:1,
> routing:1,
> csum:1,
> version:3,
> reserved:4,
> ack:1;
> /* ... */
> }
>
>
> Variable Declarations
> ---------------------
>
> In declarations, do not put any whitespace between asterisks and adjacent tokens, except for tokens that are identifiers related to types.
> (These identifiers are the names of basic types, type qualifiers, and typedef-names other than the one being declared.)
> Separate these identifiers from asterisks using a single space.
>
> Structure Declarations
>
> When declaring variables in structures, declare them sorted by use, then by size (largest to smallest), and then in alphabetical order.
> Alignment constraints may override the previous rules. The first category normally does not apply, but there are exceptions.
> Each structure element gets its own line. Try to make the structure readable by aligning the member names using spaces as shown below.
> Names following extremely long types, which therefore cannot be easily aligned with the rest, should be separated by a single space.
> struct foo {
> struct foo *next; /* List of active foo. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members */
> };
>
This is going to cause conflicts with ABI preservation. While its fine to do
when creating a new structure, you need to be very careful about shuffling
structure members around on public facing structures. Recommend not mandating
this. The other option is to allow this, but start converting public facing
api's to use opaque types with get/set routines, so that library internals can
codify the offsets to member structures appropriately.
>
> Major structures should be declared at the top of the file in which they are used, or in separate header files if they are used
> in multiple source files. Use of the structures should be by separate declarations and should be extern if they are declared in a header file.
>
> Queues
>
> Use queue(3) macros rather than rolling your own lists, whenever possible. Thus, the previous example would be better written:
> #include <sys/queue.h>
>
> struct foo {
> LIST_ENTRY(foo) link; /* Use queue macros for foo lists. */
> struct mumble amumble; /* Comment for mumble. */
> int bar; /* Try to align the comments. */
> struct verylongtypename *baz; /* Won't fit with other members */
> };
> LIST_HEAD(, foo) foohead; /* Head of global foo list. */
>
>
> DPDK also provides an optimized way to store elements in lockless rings. This should be used in all data-path code, when there are several
> consumer and/or producers to avoid locking for concurrent access.
>
You probably want to reference the api directly in some way here, so people can
go look up how to do that.
> Typedefs
>
> Avoid using typedefs for structure types. For example, use:
> struct my_struct_type {
> /* ... */
> };
>
> struct my_struct_type my_var;
>
>
> rather than:
> typedef struct my_struct_type {
> /* ... */
> } my_struct_type;
>
> my_struct_type my_var
>
>
> Typedefs are problematic because they do not properly hide their underlying type; for example, you need to know if the typedef is
> the structure itself, as shown above, or a pointer to the structure.
This isn't really true. If you make the structure opaque, so that it references
a externally declared structure, then its just a handle, and can be used without
type knoweldge (assuming the appropriate API is built for it).
> In addition, they must be declared exactly once, whereas an
> incomplete structure type can be mentioned as many times as necessary. Typedefs are difficult to use in stand-alone header files.
> The header that defines the typedef must be included before the header that uses it,
This is an excellent reason to allow header include chains.
> or by the header that uses it (which causes namespace pollution),
> or there must be a back-door mechanism for obtaining the typedef.
> NOTE #defines used instead of typedefs also are problematic (since they do not propagate the pointer type correctly due to direct text replacement).
> For example, ``#define pint int *`` does not work as expected, while ``typedef int *pint`` does work. As stated when discussing macros, typedefs
> should be preferred to macros in cases like this.
> When convention requires a typedef; make its name match the struct tag. Avoid typedefs ending in ``_t``, except as specified in Standard C or by POSIX.
> /* Make the structure name match the typedef. */
> typedef struct bar {
> int level;
> } BAR;
> typedef int foo; /* This is foo. */
> typedef const long baz; /* This is baz. */
>
So, I'd suggest removing the explination here. The rule above seems reasonably
clear (don't typedef structures), but the reasoning sort of devolves into a
discussion on why typedefs and macros are hard (but still sometimes necessecary
as referenced above). I think it would be enough to say "DPDK prefers that
structures be used to codify complex data types as a matter of style". Its
really the reason to do so.
Note also, in mandating this, you are hindering the development of API's that
use opaque data types. You can still do it of course, but you have to be sure
to define your data types as anonymous structures:
extern struct foo;
rather than new types. Not a big deal, but something to be aware of.
>
> C Function Definition, Declaration and Use
>
> Prototypes
>
> It is recommended, but not required that all functions are prototyped somewhere.
IIRC it is actually required at the moment because public functions with no
prototypes generate warnings (-Wmising-prototypes is currently implied in one of
the gcc warning options)
>
> Any function prototypes for private functions (that is, functions not used elsewhere) go at the top of the first source module. Functions
> local to one source module should be declared static.
>
> Functions used from other parts of code (external API) must be prototyped in the relevant include file.
> Function prototypes should be listed in a logical order, preferably alphabetical unless there is a compelling reason to use a different ordering.
>
> Functions that are used locally in more than one module go into a separate header file, for example, "extern.h".
>
> Do not use the ``__P`` macro.
>
Just out of curiosity, has this been a problem? __P was introduced in c89, and
I don't think I've seen it in code since C99 was released. I don't mind
including it, but I'm curious to know the history here.
> Functions that are part of an external API should be documented using Doxygen-like comments above declarations. See the Doxgen documentation topic for details.
>
> Associate names with parameter types, for example:
> void function(int fd);
>
>
> Short function prototypes should be contained on a single line. Longer prototypes, e.g. those with many parameters,
> can be split across multiple lines. Multi-line prototypes should use space-indentation to enable function parameters to line up:
> static char *function1(int _arg, const char *_arg2,
> struct foo *_arg3,
> struct bar *_arg4,
> struct baz *_arg5);
2 things:
1) Clarify the meaning of space indentation. Is it ok to use tabs and spaces
for alignment, or are only spaces allowed (note the code currently uses the
former).
2) You say function prameters should "line up", but in the example you give,
they don't. What I think you want is:
static char *function1(int _arg, const char *_arg2,
struct foo *_arg3,
struct bar *_arg4,
struct baz *_arg5);
> static void usage(void);
>
>
> Definitions
> -----------
>
> The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself.
> static char *
> function(int a1, int a2, float fl, int a4)
> {
>
The example immediately above this section doesn't follow this convention. You
should fix that.
>
> Do not declare functions inside other functions. ANSI C states that such declarations have file scope regardless of the nesting of the declaration.
> Hiding file declarations in what appears to be a local scope is undesirable and will elicit complaints from a good compiler.
>
> Old-style (K&R) function declaration should not be used, use ANSI function declarations instead as shown below. Long argument lists
> should be wrapped as described above in the function prototypes section.
> /*
> * All major routines should have a comment briefly describing what
> * they do. The comment before the "main" routine should describe
> * what the program does.
> */
> int
> main(int argc, char *argv[])
> {
> char *ep;
> long num;
> int ch;
>
>
> C Command Line Parsing
> ----------------------
>
> For consistency, getopt(3) should be used to parse options. Options should be sorted in the getopt(3) call and the switch statement,
> unless parts of the switch cascade. Elements in a switch statement that cascade should have a FALLTHROUGH comment.
> Numerical arguments should be checked for accuracy. Code that cannot be reached should have a NOTREACHED comment.
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
> switch (ch) { /* Indent the switch. */
> case 'a': /* Don't indent the case. */
> aflag = 1; /* Indent case body one tab. */
> /* FALLTHROUGH */
> case 'b':
> bflag = 1;
> break;
> case 'N':
> Nflag = 1;
> break;
> case 'n':
> num = strtol(optarg, &ep, 10);
> if (num <= 0 || *ep != '\0') {
> warnx("illegal number, -n argument -- %s",
> optarg);
> usage();
> }
> break;
> case '?':
> default:
> usage();
> /* NOTREACHED */
> }
> argc -= optind;
> argv += optind;
>
I'm not sure we need this section. I understand we have lots of examples that
use getopt, but by and large this addresses application coding, which is
somewhat outside of the purview of the DPDK. I'm wholly supportive of some
style guidelines regarding switch statements mind you, but I'm not sure we need
to mandate the usage of getopt.
>
>
>
>
> C Indentation
> -------------
>
> Control Statements and Loops
>
> Include a space after keywords (if, while, for, return, switch). Do not use braces (``{`` and ``}``) for control statements with zero or just a
> single statement, unless that statement is more than a single line in which case the braces are permitted. Forever loops are done with for statements, not while statements.
> for (p = buf; *p != '\0'; ++p)
> ; /* nothing */
> for (;;)
> stmt;
> for (;;) {
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
> }
> for (;;) {
> if (cond)
> stmt;
> }
> if (val != NULL)
> val = realloc(val, newsize);
>
>
> Parts of a for loop may be left empty. It is recommended that you do not put declarations inside blocks unless the routine is unusually complicated.
> for (; cnt < 15; cnt++) {
> stmt1;
> stmt2;
> }
>
>
> Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
> NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> If you have to wrap a long statement, put the operator at the end of the line, and indent again. For control statements (if, while, etc.),
> it is recommended that the next line be indented by two tabs, rather than one, to prevent confusion as to whether the second line of the
> control statement forms part of the statement body or not. For non-control statements, this issue does not apply, so they can be indented
> by a single tab. However, a two-tab indent is recommended in this case also to keep consistency across all statement types.
> while (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){
> x = y + z; /* control stmt body lines up with second line of */
> a = b + c; /* control statement itself if single indent used */
> }
>
> if (really_long_variable_name_1 == really_long_variable_name_2 &&
> var3 == var4){ /* two tabs used */
> x = y + z; /* statement body no longer lines up */
> a = b + c;
> }
>
> z = a + really + long + statement + that + needs +
> two + lines + gets + indented + on + the +
> second + and + subsequent + lines;
>
>
> Do not add whitespace at the end of a line.
>
> Closing and opening braces go on the same line as the else keyword. Braces that are not necessary should be left out.
> if (test)
> stmt;
> else if (bar) {
> stmt;
> stmt;
> } else
> stmt;
>
>
> Function Calls
> --------------
>
> Do not use spaces after function names. Commas should have a space after them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> error = function(a1, a2);
> if (error != 0)
> exit(error);
>
>
> Operators
> ---------
>
> Unary operators do not require spaces, binary operators do. Do not use parentheses unless they are required for precedence or unless the
> statement is confusing without them. Remember that other people may be more easily confused than you.
>
I recommend changing this such that all logical operators require spaces around
them. It simplifies the rules when writing code. Also, it appears from some
quick grepping that always including space around unary or binary logical
operations is the de-facto rule.
> Exit
>
> Exits should be 0 on success, or 1 on failure.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
> }
>
This again codifies application behavior, not coding style. I don't think we
need to mandate that as we have no purview over application behavior (example
applications being the exception here of course).
>
> Local Variables
> ---------------
>
> When declaring variables in functions, declare them sorted by size, then in alphabetical order. Multiple variables per line are OK.
> If a line overflows reuse the type keyword.
>
> Be careful to not obfuscate the code by initializing variables in the declarations, only the last variable on a line should be initialized.
> If multiple variables are to be initialised when defined, put one per line. Do not use function calls in initializers.
> int i = 0, j = 0, k = 0; /* bad, too many initializer */
>
> char a = 0; /* OK, one variable per line with initializer */
> char b = 0;
>
> float x, y = 0.0; /* OK, only last variable has initializer */
>
>
> Casts and sizeof
>
> Casts and sizeof statements are not followed by a space. Always write sizeof statements with parenthesis.
> The redundant parenthesis rules do not apply to sizeof(var) instances.
>
What redundant parenthesis rules? I presume you are referring to implied
practice of not including parenthesis in operations that don't change the order
of operations? If so, you probably want to state that clearly ealier so reader
have something to reference here.
> C Style and Conventions
>
> NULL Pointers
>
> NULL is the preferred null pointer constant. Use NULL instead of ``(type *)0`` or ``(type *)NULL`` in contexts where the compiler knows the type,
> for example, in assignments. Use ``(type *)NULL`` in other contexts, in particular for all function args.
> (Casting is essential for variadic args and is necessary for other args if the function prototype might not be in scope.) Test pointers against NULL, for example, use::
> (p = f()) == NULL
>
>
> not::
> !(p = f())
>
>
> Do not use ! for tests unless it is a boolean, for example, use::
> if (*p == '\0')
>
>
> not::
> if (!*p)
>
>
> Return Value
> ------------
>
> If possible, functions should return 0 on success and a negative value on error. The negative value should be ``-errno`` if relevant, for example, ``-EINVAL``.
>
> Routines returning ``void *`` should not have their return values cast to any pointer type.
> (Typecasting can prevent the compiler from warning about missing prototypes as any implicit definition of a function returns int - which, unlike "void *" needs a typecast to assign to a pointer variable.)
> NOTE The above rule applies to malloc, as well as to DPDK functions.
> Values in return statements should be enclosed in parentheses.
>
> Logging and Errors
> ------------------
>
> In the DPDK environment, use the logging interface provided::
> #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
>
> /* enable these logs type */
> rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
>
> /* log in debug level */
> rte_set_log_level(RTE_LOG_DEBUG);
> RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
>
> /* log in info level */
> rte_set_log_level(RTE_LOG_INFO);
> RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
>
While I'm not opposed at all to mandating a Log mechanism, it seems to me that
this is a review issue, not a coding style issue. It will also be almost
impossible to codify this requirement in a tool (how will a tool know when you
are trying to do logging, but aren't using the above log macros)?
>
> In a userland program that is not a DPDK application, use err(3) or warn(3). Do not create your own variant.
> if ((four = malloc(sizeof(struct foo))) == NULL)
> err(1, (char *)NULL);
> if ((six = (int *)overflow()) == NULL)
> errx(1, "number overflowed");
> return (eight);
> }
>
Not within our purview, don't include this. If its not part of the DPDK,
we don't get to mandate style on it.
>
> Variable Arguments List
> -----------------------
>
> Variable numbers of arguments should look like this:
> #include <stdarg.h>
>
> void
> vaf(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> STUFF;
> va_end(ap);
> /* No return needed for void functions. */
> }
>
> static void
> usage()
> {
> /* Insert an empty line if the function has no local variables. */
>
>
This isn't a coding style issue, this is just how variable arguments work in C.
I think you can remove this.
> Printf
> ------
>
> Use printf(3), not fputs(3), puts(3), putchar(3) or whatever. It is faster and usually cleaner, and helps to avoid unnecessary bugs. However, be aware of format string bugs::
> int
> main(int argc, char **argv)
> {
> if(argc != 2)
> exit(1);
> printf(argv[1]); /* bad ! */
> printf("%s", argv[1]); /* correct */
>
>
Again, not a coding style issue. There may be perfectly good reasons to use
putchar/putc/puts, I don't think our coding sytle guideline needs to restrict
their usage. Let developers review the appropriateness of the calls used in the
code.
> Usage
> -----
>
> Usage statements should look like the manual pages SYNOPSIS. The usage statement should be structured in the following order:
> 1. Options without operands come first, in alphabetical order, inside a single set of brackets (``[`` and ``]``).
> 2. Options with operands come next, also in alphabetical order, with each option and its argument inside its own pair of brackets.
> 3. Required arguments (if any) are next, listed in the order they should be specified on the command line.
> 4. Finally, any optional arguments, listed in the order they should be specified, and all inside brackets.
>
> A bar (`|') separates ``either-or`` options/arguments, and multiple options/arguments, which are specified together, are placed in a single set of brackets.
> "usage: f [-aDde] [-b b_arg] [-m m_arg] req1 req2 [opt1 [opt2]]\n"
> "usage: f [-a | -b] [-c [-dEe] [-n number]]\n"
>
> (void)fprintf(stderr, "usage: f [-ab]\n");
> exit(1);
> }
>
>
> Note that the manual page options description should list the options in pure alphabetical order. That is, without regard to
> whether an option takes arguments or not. The alphabetical ordering should take into account the case ordering shown above.
>
> Branch Prediction
> -----------------
>
> When a test is done in a critical zone (called often or in a data path) use the ``likely()`` and ``unlikely()`` macros. They are expanded
> as a compiler builtin and allow the developer to indicate if the branch is likely to be taken or not. Example:
> #include <rte_branch_prediction.h>
> if (likely(x > 1))
> do_stuff();
>
Don't need this. Developers know when to use proper branch prediction.
Mandating it can lead to inappropriate guesses, espeically if you codify this
check in a tool that mandates all conditionals be branch predicted.
The remainder of the document looks fine
Neil
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [dpdk-dev] tools brainstorming
2015-04-08 10:43 ` Butler, Siobhan A
` (6 preceding siblings ...)
2015-04-13 15:02 ` Neil Horman
@ 2015-04-16 10:49 ` Thomas Monjalon
7 siblings, 0 replies; 62+ messages in thread
From: Thomas Monjalon @ 2015-04-16 10:49 UTC (permalink / raw)
To: Butler, Siobhan A; +Cc: dev
2015-04-08 10:43, Butler, Siobhan A:
> To add to the tools brainstorming - I propose we use the following Coding
> Standards as the basis of guidelines on coding style going forward.
Thanks for proposing, it's clearly something which must be written and agreed
in coming weeks. I think it will avoid some questions/discussions and will
make contributions easier to produce and to review.
> The style outlined below is in alignment with the current convention used for
> the majority of the project.
Yes it's a sane goal.
> Any thoughts/suggestions or feedback welcome.
> Thanks
> Siobhan :)
[...]
> This document specifies the preferred style for source files in the DPDK source tree.
> It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2 Kernel Developer's Manual (see man style(9)),
URLs may be helpful:
https://www.kernel.org/doc/Documentation/CodingStyle
https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9
> but was heavily modified for the needs of the DPDK. Many of the style rules are implicit in the examples.
> Be careful to check the examples before assuming that style is silent on an issue.
[...]
> Indentation should be to no more than 3 levels deep.
Useless and too restrictive: line length recommendation should be enough.
> NOTE The above are recommendations, and not hard limits.
> However, it is expected that the recommendations should be followed in all but the rarest situations.
[...]
> Global pathnames are defined in <paths.h>. Pathnames local to the program go in "pathnames.h" in the local directory.
> #include <paths.h>
I don't understand.
> NOTE Please avoid, as much as possible, including headers from other headers file.
This was the old style. Now, the trend is to nest includes in headers to make them
self-contained:
git grep '#include.*rte' lib | grep 'h:' | sort
> Doing so should be properly explained and justified.
[...]
> Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces.
The namespace should be explicited: why it is prefixed rte_/RTE_ and not dpdk_.
> the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes;
Alignment of backslashes (and alignment in general) is hard to achieve with
tab indentation to be nice with different tab sizes.
Linux chose to fix tab at 8 spaces. I feel it is too restrictive and I'd prefer
avoiding alignments if there are different levels of indentation.
> When code is conditionally compiled using #ifdef or #if, a comment may be added following the matching #endif or #else to
> permit the reader to easily discern where conditionally compiled code regions end. This comment should be used only for
> (subjectively) long regions, regions greater than 20 lines, or where a series of nested #ifdef 's may be confusing to the reader.
Yes, excellent recommendation.
> The comment for #endif should match the expression used in the corresponding #if or #ifdef.
Matter of taste: I prefer when the #endif comment match the last condition (#if, #ifdef, #elif or #else).
> The comment for #else and #elif
> should match the inverse of the expression(s) used in the preceding #if and/or #elif statements. In the comments,
> the subexpression defined(FOO) is abbreviated as FOO. For the purposes of comments, #ifndef FOO is treated as #if !defined(FOO).
[...]
> NOTE Conditional compilation should be used only when absolutely necessary,
> as it increases the number of target binaries that need to be built and tested.
Yes :)
[...]
> In declarations, do not put any whitespace between asterisks and adjacent tokens, except for tokens that are identifiers related to types.
> (These identifiers are the names of basic types, type qualifiers, and typedef-names other than the one being declared.)
> Separate these identifiers from asterisks using a single space.
Please elaborate, it's not clear. Using "previous/following" would help.
[...]
> When declaring variables in structures, declare them sorted by use, then by size (largest to smallest), and then in alphabetical order.
Struct sorting is not so important and updates must be done at the end
if the struct is part of the API.
[...]
> It is recommended, but not required that all functions are prototyped somewhere.
No, it's simpler to maintain code without local prototypes.
Please let's stop useless declarations and let's try to implement functions in the right order.
[...]
> static void usage(void);
static void
usage(void);
> C Command Line Parsing
> ----------------------
Is this section really needed?
[...]
> Do not add whitespace at the end of a line.
Nor blank line at the end of the file.
> exit(0); /*
> * Avoid obvious comments such as
> * "Exit 0 on success."
> */
Yes please, and update or remove them when code is updated ;)
> When declaring variables in functions, declare them sorted by size, then in alphabetical order.
Disagree with such sorting. It should be dependent of the context. Just try to order at your best.
[...]
> If a line overflows reuse the type keyword.
Obviously but not clear when reading.
[...]
> Logging and Errors
> ------------------
[...]
> Variable Arguments List
> -----------------------
[...]
> Printf
> ------
[...]
> Usage
> -----
[...]
> Branch Prediction
> -----------------
[...]
These sections are useless in my opinion.
[...]
> In DPDK and DPDK applications, some code is specific to an architecture (i686, x86_64)
> or to an executive environment (bare-metal or linuxapp) and so on.
No more bare-metal.
>
> There are several ways to handle specific code:
> Use a ``#ifdef`` with the CONFIG option in the C code. This can be done when the differences are small and they can be embedded in the same C file::
> #ifdef RTE_ARCH_I686
> toto();
> #else
> titi();
> #endif
Should be avoided.
> Use the CONFIG option in the Makefile.
Should be avoided.
> This is done when the differences are more significant. In this case, the code is split into
> two separate files that are architecture or environment specific.
Most of things must be handled in EAL in specific directories (per-arch, per-OS).
[...]
> CONFIG_RTE_ARCH is a string that contains the name of the architecture.
> CONFIG_RTE_ARCH_I686, CONFIG_RTE_ARCH_X86_64 or CONFIG_RTE_ARCH_X86_64_32 are defined only if we are building for those architectures.
[...]
> CONFIG_RTE_EXEC_ENV is a string that contains the name of the executive environment.
> CONFIG_RTE_EXEC_ENV_BAREMETAL or CONFIG_RTE_EXEC_ENV_LINUXAPP are defined only if we are building for this execution environment.
Do not try to list macros, it is or will be outdated.
[...]
> Documenting Constants and Variables
> /**
> * The definition of a funny TRUE.
> */
> #define TRUE 0
>
> #define TRUE 1 /**< another way to document a macro */
It's also possible to make a comment for a group of macros:
/**@{
* Useless macros to make reader confused
*/
#define TRUE 0
#define SIZE64 32
/**@}*/
^ permalink raw reply [flat|nested] 62+ messages in thread