[v2] devtools: forbid variable declaration inside for
diff mbox series

Message ID 20200524173007.1653549-1-thomas@monjalon.net
State Accepted
Delegated to: David Marchand
Headers show
Series
  • [v2] devtools: forbid variable declaration inside for
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon May 24, 2020, 5:30 p.m. UTC
Some compilers raise an error when declaring a variable
in the middle of a function. This is a C99 allowance.
Even if DPDK switches globally to C99 or C11 standard,
the coding rules are for declarations at the beginning
of a block:
http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables

This coding style is enforced by adding a check of
the common patterns like "for (int i;"

The occurrences of the checked pattern are fixed:
	'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
In the file dpaa2_sparser.c, the fix is to remove the unused macros.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
v2:
	- check s?size_t and fix one occurence
	- fix new occurences in Windows EAL
	- test-fib has been fixed in the meantime
---
 devtools/checkpatches.sh             |  8 ++++++++
 doc/guides/prog_guide/eventdev.rst   |  6 ++++--
 drivers/common/mlx5/mlx5_devx_cmds.c |  4 ++--
 drivers/common/mlx5/mlx5_glue.c      |  3 ++-
 drivers/crypto/caam_jr/caam_jr.c     |  5 ++++-
 drivers/net/dpaa2/dpaa2_sparser.c    | 30 ----------------------------
 lib/librte_eal/windows/eal_lcore.c   |  6 ++++--
 7 files changed, 24 insertions(+), 38 deletions(-)

Comments

Stephen Hemminger May 24, 2020, 6:30 p.m. UTC | #1
On Sun, 24 May 2020 19:30:07 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
> 
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
> 
> The occurrences of the checked pattern are fixed:
> 	'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Surprised that checkpatch doesn't enforce this already.
Or maybe kernel flags are different.
David Marchand May 28, 2020, 12:30 p.m. UTC | #2
On Sun, May 24, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Some compilers raise an error when declaring a variable
> in the middle of a function. This is a C99 allowance.
> Even if DPDK switches globally to C99 or C11 standard,
> the coding rules are for declarations at the beginning
> of a block:
> http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
>
> This coding style is enforced by adding a check of
> the common patterns like "for (int i;"
>
> The occurrences of the checked pattern are fixed:
>         'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> v2:
>         - check s?size_t and fix one occurence
>         - fix new occurences in Windows EAL
>         - test-fib has been fixed in the meantime
> ---
>  devtools/checkpatches.sh             |  8 ++++++++
>  doc/guides/prog_guide/eventdev.rst   |  6 ++++--
>  drivers/common/mlx5/mlx5_devx_cmds.c |  4 ++--
>  drivers/common/mlx5/mlx5_glue.c      |  3 ++-
>  drivers/crypto/caam_jr/caam_jr.c     |  5 ++++-
>  drivers/net/dpaa2/dpaa2_sparser.c    | 30 ----------------------------
>  lib/librte_eal/windows/eal_lcore.c   |  6 ++++--
>  7 files changed, 24 insertions(+), 38 deletions(-)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 42b833e0d7..5763a7e953 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -69,6 +69,14 @@ check_forbidden_additions() { # <patch>
>                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
>                 "$1" || res=1
>
> +       # forbid variable declaration inside "for" loop
> +       awk -v FOLDERS='.' \
> +               -v EXPRESSIONS='for *\\((char|u?int|unsigned|s?size_t)' \
> +               -v RET_ON_FAIL=1 \
> +               -v MESSAGE='Declaring a variable inside for()' \
> +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> +               "$1" || res=1
> +
>         # svg figures must be included with wildcard extension
>         # because of png conversion for pdf docs
>         awk -v FOLDERS='doc' \
> diff --git a/doc/guides/prog_guide/eventdev.rst b/doc/guides/prog_guide/eventdev.rst
> index 7bcd7603b1..ccde086f63 100644
> --- a/doc/guides/prog_guide/eventdev.rst
> +++ b/doc/guides/prog_guide/eventdev.rst
> @@ -242,9 +242,10 @@ Once queues are set up successfully, create the ports as required.
>          };
>          int dev_id = 0;
>          int rx_port_id = 0;
> +        int worker_port_id;
>          int err = rte_event_port_setup(dev_id, rx_port_id, &rx_conf);
>
> -        for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> +        for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
>                 int err = rte_event_port_setup(dev_id, worker_port_id, &worker_conf);
>          }
>
> @@ -277,8 +278,9 @@ can be achieved like this:
>          uint8_t atomic_qs[] = {0, 1};
>          uint8_t single_link_q = 2;
>          uint8_t priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
> +        int worker_port_id;
>
> -        for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
> +        for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
>                  int links_made = rte_event_port_link(dev_id, worker_port_id, atomic_qs, NULL, 2);
>          }
>          int links_made = rte_event_port_link(dev_id, tx_port_id, &single_link_q, &priority, 1);
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
> index fba485e724..1c8b36c91a 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -416,7 +416,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>         uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
>         uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
>         void *hcattr;
> -       int status, syndrome, rc;
> +       int status, syndrome, rc, i;
>
>         MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
>         MLX5_SET(query_hca_cap_in, in, op_mod,
> @@ -529,7 +529,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>         attr->lro_max_msg_sz_mode = MLX5_GET
>                                         (per_protocol_networking_offload_caps,
>                                          hcattr, lro_max_msg_sz_mode);
> -       for (int i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
> +       for (i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
>                 attr->lro_timer_supported_periods[i] =
>                         MLX5_GET(per_protocol_networking_offload_caps, hcattr,
>                                  lro_timer_supported_periods[i]);
> diff --git a/drivers/common/mlx5/mlx5_glue.c b/drivers/common/mlx5/mlx5_glue.c
> index f270f677b7..563941c2df 100644
> --- a/drivers/common/mlx5/mlx5_glue.c
> +++ b/drivers/common/mlx5/mlx5_glue.c
> @@ -587,11 +587,12 @@ mlx5_glue_dv_create_flow(void *matcher,
>         return mlx5dv_dr_rule_create(matcher, match_value, num_actions,
>                                      (struct mlx5dv_dr_action **)actions);
>  #else
> +       size_t i;
>         struct mlx5dv_flow_action_attr actions_attr[8];
>
>         if (num_actions > 8)
>                 return NULL;
> -       for (size_t i = 0; i < num_actions; i++)
> +       for (i = 0; i < num_actions; i++)
>                 actions_attr[i] =
>                         *((struct mlx5dv_flow_action_attr *)(actions[i]));
>         return mlx5dv_create_flow(matcher, match_value,
> diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
> index caf2386772..77fa6ff2d3 100644
> --- a/drivers/crypto/caam_jr/caam_jr.c
> +++ b/drivers/crypto/caam_jr/caam_jr.c
> @@ -1351,6 +1351,9 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
>         struct caam_jr_session *ses;
>         struct caam_jr_op_ctx *ctx = NULL;
>         struct sec_job_descriptor_t *jobdescr __rte_unused;
> +#if CAAM_JR_DBG
> +       int i;
> +#endif
>
>         switch (op->sess_type) {
>         case RTE_CRYPTO_OP_WITH_SESSION:
> @@ -1413,7 +1416,7 @@ caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
>                         rte_pktmbuf_data_len(op->sym->m_src));
>
>         printf("\n JD before conversion\n");
> -       for (int i = 0; i < 12; i++)
> +       for (i = 0; i < 12; i++)
>                 printf("\n 0x%08x", ctx->jobdes.desc[i]);
>  #endif
>
> diff --git a/drivers/net/dpaa2/dpaa2_sparser.c b/drivers/net/dpaa2/dpaa2_sparser.c
> index 7e8fedd818..ba0d500f74 100644
> --- a/drivers/net/dpaa2/dpaa2_sparser.c
> +++ b/drivers/net/dpaa2/dpaa2_sparser.c
> @@ -145,36 +145,6 @@ struct frame_attr_ext frame_attr_ext_arr[] = {
>         /* 112 */ {NULL,                                       0, 0x0000}
>  };
>
> -#define SWAP_WORD(pr)                                          \
> -do {                                                           \
> -       for (int i = 0; i < 4 ; i++) {                          \
> -               pr[i] = pr[i] ^ pr[6 - i + 1];                  \
> -               pr[6 - i + 1] = pr[6 - i + 1] ^ pr[i];          \
> -               pr[i] = pr[i] ^ pr[6 - i + 1];                  \
> -       }                                                       \
> -} while (0)
> -
> -#define fa_print_sb()                                          \
> -do {                                                           \
> -       if (rte_cpu_to_be_32(*pdw) & frm_attr->fld_mask)        \
> -               DPAA2_PMD_DP_DEBUG("t %s : Yes", frm_attr->fld_name);   \
> -} while (0)
> -
> -#define fa_print_sb_ext()                                      \
> -do {                                                           \
> -       if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask)     \
> -               DPAA2_PMD_DP_DEBUG("\t %s : Yes",                       \
> -                         frm_attr_ext->fld_name);              \
> -} while (0)
> -
> -#define fa_print_mb_ext()                                      \
> -do {                                                           \
> -       if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask)     \
> -               DPAA2_PMD_DP_DEBUG("\t %s : 0x%02x",                    \
> -                         frm_attr_ext->fld_name,               \
> -                         rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask);\
> -} while (0)
> -
>  int dpaa2_eth_load_wriop_soft_parser(struct dpaa2_dev_priv *priv,
>                                      enum dpni_soft_sequence_dest dest)
>  {
> diff --git a/lib/librte_eal/windows/eal_lcore.c b/lib/librte_eal/windows/eal_lcore.c
> index 82ee454134..760b3fcfed 100644
> --- a/lib/librte_eal/windows/eal_lcore.c
> +++ b/lib/librte_eal/windows/eal_lcore.c
> @@ -29,6 +29,8 @@ static struct _wcpu_map {
>  void
>  eal_create_cpu_map()
>  {
> +       unsigned int socket, core;
> +
>         wcpu_map.total_procs =
>                 GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
>
> @@ -56,9 +58,9 @@ eal_create_cpu_map()
>          * equally across the sockets.
>          */
>         unsigned int lcore = 0;
> -       for (unsigned int socket = 0; socket <
> +       for (socket = 0; socket <
>                         wcpu_map.proc_sockets; ++socket) {
> -               for (unsigned int core = 0;
> +               for (core = 0;
>                         core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
>                         ++core) {
>                         wcpu_map.wlcore_map[lcore]
> --
> 2.26.2
>

Acked-by: David Marchand <david.marchand@redhat.com>
David Marchand July 3, 2020, 9:08 a.m. UTC | #3
On Thu, May 28, 2020 at 2:30 PM David Marchand
<david.marchand@redhat.com> wrote:
> On Sun, May 24, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > Some compilers raise an error when declaring a variable
> > in the middle of a function. This is a C99 allowance.
> > Even if DPDK switches globally to C99 or C11 standard,
> > the coding rules are for declarations at the beginning
> > of a block:
> > http://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
> >
> > This coding style is enforced by adding a check of
> > the common patterns like "for (int i;"
> >
> > The occurrences of the checked pattern are fixed:
> >         'for *(\(char\|u\?int\|unsigned\|s\?size_t\)'
> > In the file dpaa2_sparser.c, the fix is to remove the unused macros.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: David Marchand <david.marchand@redhat.com>

Discarded the (now obsolete) change on windows/eal.c and applied.

Patch
diff mbox series

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 42b833e0d7..5763a7e953 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -69,6 +69,14 @@  check_forbidden_additions() { # <patch>
 		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
 		"$1" || res=1
 
+	# forbid variable declaration inside "for" loop
+	awk -v FOLDERS='.' \
+		-v EXPRESSIONS='for *\\((char|u?int|unsigned|s?size_t)' \
+		-v RET_ON_FAIL=1 \
+		-v MESSAGE='Declaring a variable inside for()' \
+		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+		"$1" || res=1
+
 	# svg figures must be included with wildcard extension
 	# because of png conversion for pdf docs
 	awk -v FOLDERS='doc' \
diff --git a/doc/guides/prog_guide/eventdev.rst b/doc/guides/prog_guide/eventdev.rst
index 7bcd7603b1..ccde086f63 100644
--- a/doc/guides/prog_guide/eventdev.rst
+++ b/doc/guides/prog_guide/eventdev.rst
@@ -242,9 +242,10 @@  Once queues are set up successfully, create the ports as required.
         };
         int dev_id = 0;
         int rx_port_id = 0;
+        int worker_port_id;
         int err = rte_event_port_setup(dev_id, rx_port_id, &rx_conf);
 
-        for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
+        for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
 	        int err = rte_event_port_setup(dev_id, worker_port_id, &worker_conf);
         }
 
@@ -277,8 +278,9 @@  can be achieved like this:
         uint8_t atomic_qs[] = {0, 1};
         uint8_t single_link_q = 2;
         uint8_t priority = RTE_EVENT_DEV_PRIORITY_NORMAL;
+        int worker_port_id;
 
-        for(int worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
+        for (worker_port_id = 1; worker_port_id <= 4; worker_port_id++) {
                 int links_made = rte_event_port_link(dev_id, worker_port_id, atomic_qs, NULL, 2);
         }
         int links_made = rte_event_port_link(dev_id, tx_port_id, &single_link_q, &priority, 1);
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index fba485e724..1c8b36c91a 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -416,7 +416,7 @@  mlx5_devx_cmd_query_hca_attr(void *ctx,
 	uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
 	uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
 	void *hcattr;
-	int status, syndrome, rc;
+	int status, syndrome, rc, i;
 
 	MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
 	MLX5_SET(query_hca_cap_in, in, op_mod,
@@ -529,7 +529,7 @@  mlx5_devx_cmd_query_hca_attr(void *ctx,
 	attr->lro_max_msg_sz_mode = MLX5_GET
 					(per_protocol_networking_offload_caps,
 					 hcattr, lro_max_msg_sz_mode);
-	for (int i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
+	for (i = 0 ; i < MLX5_LRO_NUM_SUPP_PERIODS ; i++) {
 		attr->lro_timer_supported_periods[i] =
 			MLX5_GET(per_protocol_networking_offload_caps, hcattr,
 				 lro_timer_supported_periods[i]);
diff --git a/drivers/common/mlx5/mlx5_glue.c b/drivers/common/mlx5/mlx5_glue.c
index f270f677b7..563941c2df 100644
--- a/drivers/common/mlx5/mlx5_glue.c
+++ b/drivers/common/mlx5/mlx5_glue.c
@@ -587,11 +587,12 @@  mlx5_glue_dv_create_flow(void *matcher,
 	return mlx5dv_dr_rule_create(matcher, match_value, num_actions,
 				     (struct mlx5dv_dr_action **)actions);
 #else
+	size_t i;
 	struct mlx5dv_flow_action_attr actions_attr[8];
 
 	if (num_actions > 8)
 		return NULL;
-	for (size_t i = 0; i < num_actions; i++)
+	for (i = 0; i < num_actions; i++)
 		actions_attr[i] =
 			*((struct mlx5dv_flow_action_attr *)(actions[i]));
 	return mlx5dv_create_flow(matcher, match_value,
diff --git a/drivers/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
index caf2386772..77fa6ff2d3 100644
--- a/drivers/crypto/caam_jr/caam_jr.c
+++ b/drivers/crypto/caam_jr/caam_jr.c
@@ -1351,6 +1351,9 @@  caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
 	struct caam_jr_session *ses;
 	struct caam_jr_op_ctx *ctx = NULL;
 	struct sec_job_descriptor_t *jobdescr __rte_unused;
+#if CAAM_JR_DBG
+	int i;
+#endif
 
 	switch (op->sess_type) {
 	case RTE_CRYPTO_OP_WITH_SESSION:
@@ -1413,7 +1416,7 @@  caam_jr_enqueue_op(struct rte_crypto_op *op, struct caam_jr_qp *qp)
 			rte_pktmbuf_data_len(op->sym->m_src));
 
 	printf("\n JD before conversion\n");
-	for (int i = 0; i < 12; i++)
+	for (i = 0; i < 12; i++)
 		printf("\n 0x%08x", ctx->jobdes.desc[i]);
 #endif
 
diff --git a/drivers/net/dpaa2/dpaa2_sparser.c b/drivers/net/dpaa2/dpaa2_sparser.c
index 7e8fedd818..ba0d500f74 100644
--- a/drivers/net/dpaa2/dpaa2_sparser.c
+++ b/drivers/net/dpaa2/dpaa2_sparser.c
@@ -145,36 +145,6 @@  struct frame_attr_ext frame_attr_ext_arr[] = {
 	/* 112 */ {NULL,                                       0, 0x0000}
 };
 
-#define SWAP_WORD(pr)						\
-do {								\
-	for (int i = 0; i < 4 ; i++) {				\
-		pr[i] = pr[i] ^ pr[6 - i + 1];			\
-		pr[6 - i + 1] = pr[6 - i + 1] ^ pr[i];		\
-		pr[i] = pr[i] ^ pr[6 - i + 1];			\
-	}							\
-} while (0)
-
-#define fa_print_sb()						\
-do {								\
-	if (rte_cpu_to_be_32(*pdw) & frm_attr->fld_mask)	\
-		DPAA2_PMD_DP_DEBUG("t %s : Yes", frm_attr->fld_name);	\
-} while (0)
-
-#define fa_print_sb_ext()					\
-do {								\
-	if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask)	\
-		DPAA2_PMD_DP_DEBUG("\t %s : Yes",			\
-			  frm_attr_ext->fld_name);		\
-} while (0)
-
-#define fa_print_mb_ext()					\
-do {								\
-	if (rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask)	\
-		DPAA2_PMD_DP_DEBUG("\t %s : 0x%02x",			\
-			  frm_attr_ext->fld_name,		\
-			  rte_cpu_to_be_16(*pw) & frm_attr_ext->fld_mask);\
-} while (0)
-
 int dpaa2_eth_load_wriop_soft_parser(struct dpaa2_dev_priv *priv,
 				     enum dpni_soft_sequence_dest dest)
 {
diff --git a/lib/librte_eal/windows/eal_lcore.c b/lib/librte_eal/windows/eal_lcore.c
index 82ee454134..760b3fcfed 100644
--- a/lib/librte_eal/windows/eal_lcore.c
+++ b/lib/librte_eal/windows/eal_lcore.c
@@ -29,6 +29,8 @@  static struct _wcpu_map {
 void
 eal_create_cpu_map()
 {
+	unsigned int socket, core;
+
 	wcpu_map.total_procs =
 		GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
 
@@ -56,9 +58,9 @@  eal_create_cpu_map()
 	 * equally across the sockets.
 	 */
 	unsigned int lcore = 0;
-	for (unsigned int socket = 0; socket <
+	for (socket = 0; socket <
 			wcpu_map.proc_sockets; ++socket) {
-		for (unsigned int core = 0;
+		for (core = 0;
 			core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
 			++core) {
 			wcpu_map.wlcore_map[lcore]