Message ID | 20230926192400.19366-1-petr@tesarici.cz |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2207176vqu; Tue, 26 Sep 2023 14:37:17 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHhdEBTLyYXyh4CydtSH2LsGAAsvtI4rITYMyIb0iWHnEe871o547E7cduhqf6c/YerYEwj X-Received: by 2002:a05:6a20:a120:b0:148:f952:552b with SMTP id q32-20020a056a20a12000b00148f952552bmr85777pzk.51.1695764237111; Tue, 26 Sep 2023 14:37:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695764237; cv=none; d=google.com; s=arc-20160816; b=lSP004X02fWlK9GMnZhLQkNcVKnLn7J0feZ+x13Wu6PLEe1fmaoYqpOr788xC3tnwc tJMbIYdwvfooSs9SnNHCM8T2NJGAgnYjfz8QCy+dYshgpQt3LFOsX96vfP3KuwM0g3jb MK6T2lHI55hRgQDEI1Wlcb60z3MyaSDGSJPJY9W/ULkB2TvIk0oOQ3QBEvyTnV9ItW8D DG0ZGJs2BwyP2EaBRmhY63yoxGrLO1GMni3ixBXLArFiO9u+YDo4nP03i5m6/+SaVAVd 23VB8SUu5HstrKmadUfpSWNV59YaNJDHTTcLJ8DpSnPxrRhBjPD4Tb334FwzTa3Y4s1r dO+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=wN0W47RTDOU1Yq1+TeE4qrYFkVwmz5gMq/wd9D9Sc94=; fh=oevolkDE1K0CeTYR3D1eCklAdNWfou4M43LL7npywNg=; b=uuBSDCC5yyXKJEvOli6nViGqbug696e6KRGZoJDbTatElqKslTVuwVHHbZWW0KzTAi Y2ZD1d3YZV4lBa2d+JX41c/AKRsynkoQwBGsqZpvTZFkjBfjI+kUdzULkfOJI/PYXbVq beeUQ4uSKnJwbfbmnynxS6440dNf9etm9lD7xjbo3An3FW+iXWBlygDukgSMQSpUOdJa 0WMhu4wUnQxc4QP94rA6wpr1gJs/G4eWKDSqljQGyjMNTH8KFlGsb/HmwQCfQonaVEKb /bjQELGRxyTkxhQD3082Mku5Q5+soGb15Tkdzv8cxDDEtmIGYEGXB8kfI8rV9L3koDlR DvzA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@tesarici.cz header.s=mail header.b=XQLB8Y3k; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=tesarici.cz Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id x1-20020a170902a38100b001bf20c80685si12786167pla.125.2023.09.26.14.37.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 14:37:17 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@tesarici.cz header.s=mail header.b=XQLB8Y3k; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=tesarici.cz Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 415F5802C91C; Tue, 26 Sep 2023 12:24:25 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235694AbjIZTYR (ORCPT <rfc822;pwkd43@gmail.com> + 28 others); Tue, 26 Sep 2023 15:24:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57800 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235738AbjIZTYO (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 26 Sep 2023 15:24:14 -0400 Received: from bee.tesarici.cz (bee.tesarici.cz [77.93.223.253]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 44CDA180 for <linux-kernel@vger.kernel.org>; Tue, 26 Sep 2023 12:24:08 -0700 (PDT) Received: from meshulam.tesarici.cz (dynamic-2a00-1028-83b8-1e7a-4427-cc85-6706-c595.ipv6.o2.cz [IPv6:2a00:1028:83b8:1e7a:4427:cc85:6706:c595]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by bee.tesarici.cz (Postfix) with ESMTPSA id 8CEA3181673; Tue, 26 Sep 2023 21:24:06 +0200 (CEST) Authentication-Results: mail.tesarici.cz; dmarc=fail (p=none dis=none) header.from=tesarici.cz DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tesarici.cz; s=mail; t=1695756246; bh=XruJiBHgq+nHTppk3DIZz92BSsLm3t0Pi2qS8t6/ImM=; h=From:To:Cc:Subject:Date:From; b=XQLB8Y3klEtZzr9+TDLL/x32AzWd7ykFiuSofy4z0VCJU+b/r9+4WWJ+2w01VsC5W XyPXB355gORURqgEPW+aMzIkVEwmK5LpFQCgQDzVOKbgXUQxxAJLnPjipFG9SnPdQx tAgMb9Xb1aZ5kE0MSfegv/aJy2i2FnKzemETj/usdg2TfyY0vldwUpUCLiiSmcfMxx MeWeD3RAGmb9tY/UdkgtKCB+S/pNsaytH29cqjRDYxWRdLHYJubZqARx6HJAk5iXc2 /LE6Cg6pg/AUmMpWOcv5uL96piA4+keKQ9mbYMIVytbODjEYWsAHoFOZkEZuMIhTob g0I+slAax448A== From: Petr Tesarik <petr@tesarici.cz> To: Andy Whitcroft <apw@canonical.com>, Joe Perches <joe@perches.com>, Dwaipayan Ray <dwaipayanray1@gmail.com>, Lukas Bulwahn <lukas.bulwahn@gmail.com>, linux-kernel@vger.kernel.org (open list) Cc: Catalin Marinas <catalin.marinas@arm.com>, Petr Tesarik <petr@tesarici.cz> Subject: [PATCH v2] checkpatch: warn about multi-line comments without an empty /* line Date: Tue, 26 Sep 2023 21:24:00 +0200 Message-ID: <20230926192400.19366-1-petr@tesarici.cz> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 26 Sep 2023 12:24:25 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778131548226789344 X-GMAIL-MSGID: 1778137680617345151 |
Series |
[v2] checkpatch: warn about multi-line comments without an empty /* line
|
|
Commit Message
Petr Tesařík
Sept. 26, 2023, 7:24 p.m. UTC
According to Documentation/process/coding-style.rst, the preferred style
for multi-line comments outside net/ and drivers/net/ is:
.. code-block:: c
/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/
Signed-off-by: Petr Tesarik <petr@tesarici.cz>
---
scripts/checkpatch.pl | 8 ++++++++
1 file changed, 8 insertions(+)
Comments
On Tue, 26 Sep 2023 12:56:33 -0700 Joe Perches <joe@perches.com> wrote: > On Tue, 2023-09-26 at 21:24 +0200, Petr Tesarik wrote: > > According to Documentation/process/coding-style.rst, the preferred style > > for multi-line comments outside net/ and drivers/net/ is: > > > > .. code-block:: c > > > > /* > > * This is the preferred style for multi-line > > * comments in the Linux kernel source code. > > * Please use it consistently. > > * > > * Description: A column of asterisks on the left side, > > * with beginning and ending almost-blank lines. > > */ > > > > Signed-off-by: Petr Tesarik <petr@tesarici.cz> > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -4006,6 +4006,14 @@ sub process { > > "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev); > > } > > > > +# Non-networking without an initial /* > > + if ($realfile !~ m@^(drivers/net/|net/)@ && > > + $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ && > > + $rawline =~ /^\+[ \t]*\*/) { > > + WARN("BLOCK_COMMENT_STYLE", > > + "multi-line block comments should start with an empty /* line\n" . $hereprev); > > + } > > + > > # Block comments use * on subsequent lines > > if ($prevline =~ /$;[ \t]*$/ && #ends in comment > > $prevrawline =~ /^\+.*?\/\*/ && #starting /* > > Still nack. Too many existing instances. > > $ git grep '/\*.*' -- '*.[ch]' | \ > grep -v '/\*.*\*/' | \ > grep -v -P "/\*\s*$" | \ > grep -v '/\*\*' | \ > grep -v "SPDX-License" | \ > grep -v -P '^drivers/net|^net/' | \ > wc -l > 51834 Um. Not everything that is currently found in the source tree would be accepted as new code by today's standards... As it stands, the script checks the special case for net/ and drivers/net/ but does not help prevent unnecessary respins, like this one: https://lore.kernel.org/linux-iommu/ZRMgObTMkfq8Bjbe@arm.com/ OTOH if we don't want to warn about multi-line comments, maybe we don't want to call it the preferred style, and the corresponding paragraph should be removed from coding-style.rst. Do you suggest I try that instead? Petr T
Hi Joe, On Tue, 26 Sep 2023 13:39:34 -0700 Joe Perches <joe@perches.com> wrote: > On Tue, 2023-09-26 at 22:16 +0200, Petr Tesařík wrote: > > On Tue, 26 Sep 2023 12:56:33 -0700 > > Joe Perches <joe@perches.com> wrote: > > > > > On Tue, 2023-09-26 at 21:24 +0200, Petr Tesarik wrote: > > > > According to Documentation/process/coding-style.rst, the preferred style > > > > for multi-line comments outside net/ and drivers/net/ is: > > > > > > > > .. code-block:: c > > > > > > > > /* > > > > * This is the preferred style for multi-line > > > > * comments in the Linux kernel source code. > > > > * Please use it consistently. > > > > * > > > > * Description: A column of asterisks on the left side, > > > > * with beginning and ending almost-blank lines. > > > > */ > > > > > > > > Signed-off-by: Petr Tesarik <petr@tesarici.cz> > > > [] > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > [] > > > > @@ -4006,6 +4006,14 @@ sub process { > > > > "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev); > > > > } > > > > > > > > +# Non-networking without an initial /* > > > > + if ($realfile !~ m@^(drivers/net/|net/)@ && > > > > + $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ && > > > > + $rawline =~ /^\+[ \t]*\*/) { > > > > + WARN("BLOCK_COMMENT_STYLE", > > > > + "multi-line block comments should start with an empty /* line\n" . $hereprev); > > > > + } > > > > + > > > > # Block comments use * on subsequent lines > > > > if ($prevline =~ /$;[ \t]*$/ && #ends in comment > > > > $prevrawline =~ /^\+.*?\/\*/ && #starting /* > > > > > > Still nack. Too many existing instances. > > > > > > $ git grep '/\*.*' -- '*.[ch]' | \ > > > grep -v '/\*.*\*/' | \ > > > grep -v -P "/\*\s*$" | \ > > > grep -v '/\*\*' | \ > > > grep -v "SPDX-License" | \ > > > grep -v -P '^drivers/net|^net/' | \ > > > wc -l > > > 51834 > > > > Um. Not everything that is currently found in the source tree would be > > accepted as new code by today's standards... > > > > As it stands, the script checks the special case for net/ and > > drivers/net/ but does not help prevent unnecessary respins, like this > > one: > > > > https://lore.kernel.org/linux-iommu/ZRMgObTMkfq8Bjbe@arm.com/ > > > > OTOH if we don't want to warn about multi-line comments, maybe we don't > > want to call it the preferred style, and the corresponding paragraph > > should be removed from coding-style.rst. Do you suggest I try that > > instead? > > If you really want to bring it up as a coding style issue > go ahead, but consider that the link above is a 'nitpick' > and not an actual issue. I don't want to start a flamewar. In fact, I'm quite relaxed about coding style, so I'm not the right person to propose changes. However, if enough people believe that most instances found by your grep command are perfectly valid style, then the text in coding-style.rst does not match reality... > If you _really_ want, but I am not at all sure it's useful, > I suggest you change the message to a CHK so that it is only > emitted with --strict and not a WARN and only emit anything > when the thing being scanned is a patch and _not_ a file. OK, I'm trying to understand what it means when the script WARNs about something. IIUC these are just warnings about _possible_ coding style violations; authors and maintainers can choose to ignore them (although I know some maintainers are more strict than others on this matter). And then there is existing code. On the one hand, I get a warning when for BUG_ON(), although there are many existing BUG_ON() instances: $ git grep -w BUG_ON | wc -l 7561 Yet, there is a check for BUG_ON(). I get a warning even when I merely modify the argument of a BUG_ON(), e.g. as a result of renaming a struct member. This makes me believe that warnings from checkpatch.pl are not meant to be enforced. On the other hand, you (the maintainer) are not sure if it's useful to add a warning about block comment style, where the official guidelines has explicitly asked authors to "use it consistently" since 2006 when Randy Dunlap wrote commit b3fc9941fbc6e ("CodingStyle updates"). > Something like: > > # Non-networking without an initial /* > if (!$file && > $realfile !~ m@^(?:drivers/net/|net/)@ && > $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ && > $rawline =~ /^\+[ \t]*\*/) { > CHK("BLOCK_COMMENT_STYLE", > "multi-line block comments should start with an empty /* line\n" . $hereprev); > > But you still want to examine some of the false positives > this would create like: > > /* ------------------------ > * block message > * ------------------------ */ OK, I can see this style is often used to mark larger sections. > and > > struct foo { > int a; /* some desriptor */ > int b; /* some descriptor > on multiple lines */ AFAICS this would not trigger a warning in my proposed code, because the first line of the comment is preceded by non-white-space characters. I have also seen a lot of instances at the very beginning of a file. What about skipping the check if a block comment is followed by an empty line? I was also thinking about minus signs, but I'm not sure if the following instance is considered perfectly acceptable: /* -- Watchdog Timeout -- * Bit 0-6 (Reserved) * Bit 7 WDT Time-out Value Units Select * (0 = Minutes, 1 = Seconds) */ outb(timeout_unit, sch311x_wdt_data.runtime_reg + WDT_TIME_OUT); (found in drivers/watchdog/sch311x_wdt.c) All I want to achieve is that the check script somehow helps me keep a consistent style both in network code and non-network code. It's difficult for me as a human to keep paying attention to all these little details and subtle differences among maintainers, but at least some people do care about them. An automated check might help productivity IMO. Petr T
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 7d16f863edf1..e48e7b4d08c0 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4006,6 +4006,14 @@ sub process { "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev); } +# Non-networking without an initial /* + if ($realfile !~ m@^(drivers/net/|net/)@ && + $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ && + $rawline =~ /^\+[ \t]*\*/) { + WARN("BLOCK_COMMENT_STYLE", + "multi-line block comments should start with an empty /* line\n" . $hereprev); + } + # Block comments use * on subsequent lines if ($prevline =~ /$;[ \t]*$/ && #ends in comment $prevrawline =~ /^\+.*?\/\*/ && #starting /*