[PATCH 2/2] Task 2: checkpath.pl documentation mentorship: remove semicolon warnings.

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Aug 2 06:57:08 UTC 2021


On Mon, Aug 2, 2021 at 1:09 AM Rahul Balaji <rahulbalaji78 at gmail.com> wrote:
>
> i changed the lines such that they no longer evoke the (ONE_SEMICOLON) warning
> where possible.
>
> there were 7 files with true positive violation.
>
> drivers/mtd/ubi/block.c:408: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> drivers/gpu/drm/nouveau/nouveau_gem.c:342: WARNING:ONE_SEMICOLON:
> Statements terminations use 1 semicolon
>
> fs/block_dev.c:1269: WARNING:ONE_SEMICOLON: Statements terminations
> use 1 semicolon
>
> (for fs/block_dev.c, the error was actually on line 1271 for me, not on 1269)
>
> include/trace/events/iocost.h:163: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> net/sched/cls_rsvp.h:231: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> net/sched/cls_u32.c:782: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> drivers/net/ethernet/mediatek/mtk_star_emac.c:1089:
> WARNING:ONE_SEMICOLON: Statements terminations use 1 semicolon
>
> the rest of the files given below i think are false positives, as they repeatedly use ";;" in their code.
> so i guess it is necessary in their case.

Can you explain your guess? Why should the number of occurrences in
one file be an indicator for a true positive or false positive?

>
> arch/ia64/include/asm/mca_asm.h:139: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/include/asm/mca_asm.h:213: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/include/asm/native/inst.h:79: WARNING:ONE_SEMICOLON:
> Statements terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:14: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:151: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:213: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
> ---
>  drivers/gpu/drm/nouveau/nouveau_gem.c         | 2 +-
>  drivers/net/ethernet/mediatek/mtk_star_emac.c | 2 +-
>  fs/block_dev.c                                | 2 +-
>  include/trace/events/iocost.h                 | 2 +-
>  net/sched/cls_rsvp.h                          | 2 +-
>  net/sched/cls_u32.c                           | 2 +-
>  6 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 5b27845075a1..d476940ee97c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -339,7 +339,7 @@ nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains,
>         struct ttm_buffer_object *bo = &nvbo->bo;
>         uint32_t domains = valid_domains & nvbo->valid_domains &
>                 (write_domains ? write_domains : read_domains);
> -       uint32_t pref_domains = 0;;
> +       uint32_t pref_domains = 0;
>
>         if (!domains)
>                 return -EINVAL;
> diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> index 96d2891f1675..0a445dde67ae 100644
> --- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
> +++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> @@ -1086,7 +1086,7 @@ static void mtk_star_tx_complete_all(struct mtk_star_priv *priv)
>
>         spin_lock(&priv->lock);
>
> -       for (pkts_compl = 0, bytes_compl = 0;;
> +       for (pkts_compl = 0, bytes_compl = 0;
>              pkts_compl++, bytes_compl += ret, wake = true) {

Was this driver buggy?

Do you intend to modify the behavior of this driver?

Are you sure that your change does not modify the behaviour of this
driver? Is it a purely syntactic change here? Does it even compile
after your change?

>                 if (!mtk_star_ring_descs_available(ring))
>                         break;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9ef4f1fc2cb0..61b938fa5d5b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1268,7 +1268,7 @@ static int blkdev_get_whole(struct block_device *bdev, fmode_t mode)
>         if (test_bit(GD_NEED_PART_SCAN, &disk->state))
>                 bdev_disk_changed(disk, false);
>         bdev->bd_openers++;
> -       return 0;;
> +       return 0;
>  }
>
>  static void blkdev_put_whole(struct block_device *bdev, fmode_t mode)
> diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
> index e282ce02fa2d..6d1626e7a4ce 100644
> --- a/include/trace/events/iocost.h
> +++ b/include/trace/events/iocost.h
> @@ -160,7 +160,7 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
>
>         TP_fast_assign(
>                 __assign_str(devname, ioc_name(ioc));
> -               __entry->old_vrate = atomic64_read(&ioc->vtime_rate);;
> +               __entry->old_vrate = atomic64_read(&ioc->vtime_rate);
>                 __entry->new_vrate = new_vrate;
>                 __entry->busy_level = ioc->busy_level;
>                 __entry->read_missed_ppm = missed_ppm[READ];
> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
> index 27a4b6dbcf57..b7929dfcca1f 100644
> --- a/net/sched/cls_rsvp.h
> +++ b/net/sched/cls_rsvp.h
> @@ -228,7 +228,7 @@ static void rsvp_replace(struct tcf_proto *tp, struct rsvp_filter *n, u32 h)
>
>         for (s = rtnl_dereference(head->ht[h1]); s;
>              s = rtnl_dereference(s->next)) {
> -               for (ins = &s->ht[h2], pins = rtnl_dereference(*ins); ;
> +               for (ins = &s->ht[h2], pins = rtnl_dereference(*ins);

Again:

Was this code buggy?

Do you intend to modify the behavior of this code?

Are you sure that your change does not modify the behaviour of this
code? Is it a purely syntactic change here? Does it even compile after
your change?


>                      ins = &pins->next, pins = rtnl_dereference(*ins)) {
>                         if (pins->handle == h) {
>                                 RCU_INIT_POINTER(n->next, pins->next);
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 6e1abe805448..290359f6ce6f 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -779,7 +779,7 @@ static void u32_replace_knode(struct tcf_proto *tp, struct tc_u_common *tp_c,
>         /* The node must always exist for it to be replaced if this is not the
>          * case then something went very wrong elsewhere.
>          */
> -       for (pins = rtnl_dereference(*ins); ;
> +       for (pins = rtnl_dereference(*ins);
>              ins = &pins->next, pins = rtnl_dereference(*ins))

Again:

Was this code buggy?

Do you intend to modify the behavior of this code?

Are you sure that your change does not modify the behaviour of this
code? Is it a purely syntactic change here? Does it even compile after
your change?

>                 if (pins->handle == n->handle)
>                         break;
> --
> 2.25.1
>

Overall: please think more critically about what code you are
modifying and why. Just that checkpatch mentions some pattern, does
not mean that the change is always reasonable (Think for yourself!).
Maybe, you can think about a change to the rule so that a certain
pattern is not indicated anymore... and after some discussion, show
that you can IMPLEMENT such change to checkpatch. You want to
contribute improvements to checkpatch and WE WANT you to contribute as
well.

Lukas


More information about the Linux-kernel-mentees mailing list