DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev <dev@dpdk.org>,
	Vladimir Medvedkin <vladimir.medvedkin@intel.com>,
	 John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	 Matan Azrad <matan@mellanox.com>,
	Shahaf Shuler <shahafs@mellanox.com>,
	 Viacheslav Ovsiienko <viacheslavo@mellanox.com>,
	Gagandeep Singh <g.singh@nxp.com>,
	 Hemant Agrawal <hemant.agrawal@nxp.com>,
	Sachin Saxena <sachin.saxena@nxp.com>,
	Harini Ramakrishnan <harini.ramakrishnan@microsoft.com>,
	Omar Cardona <ocardona@microsoft.com>,
	 Pallavi Kadam <pallavi.kadam@intel.com>,
	Ranjit Menon <ranjit.menon@intel.com>
Subject: Re: [dpdk-dev] [PATCH] devtools: forbid variable declaration inside for
Date: Mon, 23 Mar 2020 14:40:45 +0100
Message-ID: <CAJFAV8xbz+7gRO+4oTaM0MF4+r69w_2c+oSixH_44Zn1Rs5q4A@mail.gmail.com> (raw)
In-Reply-To: <20200217222654.2218926-1-thomas@monjalon.net>

On Mon, Feb 17, 2020 at 11:27 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\)'

You missed size_t:
drivers/common/mlx5/mlx5_glue.c:        for (size_t i = 0; i < num_actions; i++)

> In the file dpaa2_sparser.c, the fix is to remove the unused macros.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test-fib/main.c                    |  3 ++-
>  devtools/checkpatches.sh               |  8 +++++++
>  doc/guides/prog_guide/eventdev.rst     |  6 ++++--
>  drivers/common/mlx5/mlx5_devx_cmds.c   |  4 ++--
>  drivers/crypto/caam_jr/caam_jr.c       |  5 ++++-
>  drivers/net/dpaa2/dpaa2_sparser.c      | 30 --------------------------
>  lib/librte_eal/windows/eal/eal_lcore.c |  9 ++++----
>  7 files changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/app/test-fib/main.c b/app/test-fib/main.c
> index 3c29ca461e..16d40a6a02 100644
> --- a/app/test-fib/main.c
> +++ b/app/test-fib/main.c
> @@ -363,6 +363,7 @@ static void
>  gen_random_rt_6(struct rt_rule_6 *rt, int nh_sz)
>  {
>         uint32_t i, j, k = 0;
> +       int a;
>
>         if (config.nb_routes_per_depth[0] != 0) {
>                 memset(rt[k].addr, 0, 16);
> @@ -370,7 +371,7 @@ gen_random_rt_6(struct rt_rule_6 *rt, int nh_sz)
>                 rt[k++].nh = rte_rand() & get_max_nh(nh_sz);
>         }
>
> -       for (int a = 0; a < 4; a++) {
> +       for (a = 0; a < 4; a++) {
>                 for (i = 1; i <= 32; i++) {
>                         uint32_t rnd;
>                         double edge = 0;
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index b16bace927..55e7ba05a0 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -61,6 +61,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
> +

Just a note, base drivers imported "as is" will probably trigger warnings.


>         # 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 d960bc93de..ae9dba2e0c 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -412,7 +412,7 @@ mlx5_devx_cmd_query_hca_attr(struct ibv_context *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,
> @@ -521,7 +521,7 @@ mlx5_devx_cmd_query_hca_attr(struct ibv_context *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/crypto/caam_jr/caam_jr.c b/drivers/crypto/caam_jr/caam_jr.c
> index 8aaa3d45f6..9aa64ab23f 100644
> --- a/drivers/crypto/caam_jr/caam_jr.c
> +++ b/drivers/crypto/caam_jr/caam_jr.c
> @@ -1353,6 +1353,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:
> @@ -1415,7 +1418,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/eal_lcore.c b/lib/librte_eal/windows/eal/eal_lcore.c
> index b3a6c63afa..8ee1d6fb78 100644
> --- a/lib/librte_eal/windows/eal/eal_lcore.c
> +++ b/lib/librte_eal/windows/eal/eal_lcore.c
> @@ -27,6 +27,8 @@ static struct _wcpu_map {
>  void
>  eal_create_cpu_map()
>  {
> +       unsigned int socket, core, lcore;
> +
>         wcpu_map.total_procs =
>                 GetActiveProcessorCount(ALL_PROCESSOR_GROUPS);
>
> @@ -53,10 +55,9 @@ eal_create_cpu_map()
>          * across the logical cores. For now, split the cores
>          * equally across the sockets.
>          */
> -       unsigned int lcore = 0;
> -       for (unsigned int socket = 0; socket <
> -                       wcpu_map.proc_sockets; ++socket) {
> -               for (unsigned int core = 0;
> +       lcore = 0;
> +       for (socket = 0; socket < wcpu_map.proc_sockets; ++socket) {
> +               for (core = 0;
>                         core < (wcpu_map.proc_cores / wcpu_map.proc_sockets);
>                         ++core) {
>                         wcpu_map.wlcore_map[lcore]
> --
> 2.25.0
>


LGTM once size_t is fixed.

-- 
David Marchand


  reply	other threads:[~2020-03-23 13:41 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-17 22:26 Thomas Monjalon
2020-03-23 13:40 ` David Marchand [this message]
2020-03-23 13:47   ` Thomas Monjalon
2020-03-23 13:59 ` Bruce Richardson
2020-03-23 16:12   ` Thomas Monjalon
2020-05-24 17:30 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2020-05-24 18:30   ` Stephen Hemminger
2020-05-28 12:30   ` David Marchand
2020-07-03  9:08     ` David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJFAV8xbz+7gRO+4oTaM0MF4+r69w_2c+oSixH_44Zn1Rs5q4A@mail.gmail.com \
    --to=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=g.singh@nxp.com \
    --cc=harini.ramakrishnan@microsoft.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=matan@mellanox.com \
    --cc=ocardona@microsoft.com \
    --cc=pallavi.kadam@intel.com \
    --cc=ranjit.menon@intel.com \
    --cc=sachin.saxena@nxp.com \
    --cc=shahafs@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@mellanox.com \
    --cc=vladimir.medvedkin@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git