[PATCH] checkpatch: prevent reporting C99_COMMENTS error for SPDX tag in .c file

[PATCH] checkpatch: prevent reporting C99_COMMENTS error for SPDX tag in .c file

[Quentin Monnet]

When checkpatch.pl is invoked with "--ignore C99_COMMENT_TOLERANCE", it
reports C99-style comments found in the code, by matching on the
double-slash pattern "//". This includes the leading slashes before the
SPDX tags that are now used in a majority of C files.

Such tags are commented with the double-slash on purpose, and should not
trigger errors from checkpatch. Let's ignore them when searching for
C99-style comments to report.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 scripts/checkpatch.pl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3cacc122c528..67f350c580ea 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3983,7 +3983,10 @@ sub process {
 		}
 
 # no C99 // comments
-		if ($line =~ m{//}) {
+		if ($line =~ m{//} &&
+		    !($rawline =~ m{// SPDX-License-Identifier:} &&
+		      $realfile =~ /\.c$/ &&
+		      $realline == $checklicenseline)) {
 			if (ERROR("C99_COMMENTS",
 				  "do not use C99 // comments\n" . $herecurr) &&
 			    $fix) {
-- 
2.20.1

Re: [PATCH] checkpatch: prevent reporting C99_COMMENTS error for SPDX tag in .c file

[Joe Perches]

(adding Vadem Bendebury who added the tolerance test)

On Tue, 2020-06-30 at 15:35 +0100, Quentin Monnet wrote:
> When checkpatch.pl is invoked with "--ignore C99_COMMENT_TOLERANCE", it
> reports C99-style comments found in the code, by matching on the
> double-slash pattern "//". This includes the leading slashes before the
> SPDX tags that are now used in a majority of C files.
> 
> Such tags are commented with the double-slash on purpose, and should not
> trigger errors from checkpatch. Let's ignore them when searching for
> C99-style comments to report.
> 
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>

I think this unnecessary as perhaps those that want no
c99 comments likely _really_ want no c99 comments.

> ---
>  scripts/checkpatch.pl | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3cacc122c528..67f350c580ea 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3983,7 +3983,10 @@ sub process {
>  		}
>  
>  # no C99 // comments
> -		if ($line =~ m{//}) {
> +		if ($line =~ m{//} &&
> +		    !($rawline =~ m{// SPDX-License-Identifier:} &&
> +		      $realfile =~ /\.c$/ &&
> +		      $realline == $checklicenseline)) {
>  			if (ERROR("C99_COMMENTS",
>  				  "do not use C99 // comments\n" . $herecurr) &&
>  			    $fix) {

Re: [PATCH] checkpatch: prevent reporting C99_COMMENTS error for SPDX tag in .c file

[Vadim Bendebury]

On Tue, Jun 30, 2020 at 7:47 AM Joe Perches <joe@perches.com> wrote:
>
> (adding Vadem Bendebury who added the tolerance test)
>
> On Tue, 2020-06-30 at 15:35 +0100, Quentin Monnet wrote:
> > When checkpatch.pl is invoked with "--ignore C99_COMMENT_TOLERANCE", it
> > reports C99-style comments found in the code, by matching on the
> > double-slash pattern "//". This includes the leading slashes before the
> > SPDX tags that are now used in a majority of C files.
> >
> > Such tags are commented with the double-slash on purpose, and should not
> > trigger errors from checkpatch. Let's ignore them when searching for
> > C99-style comments to report.
> >
> > Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>
> I think this unnecessary as perhaps those that want no
> c99 comments likely _really_ want no c99 comments.
>
> > ---
> >  scripts/checkpatch.pl | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 3cacc122c528..67f350c580ea 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3983,7 +3983,10 @@ sub process {
> >               }
> >
> >  # no C99 // comments
> > -             if ($line =~ m{//}) {
> > +             if ($line =~ m{//} &&
> > +                 !($rawline =~ m{// SPDX-License-Identifier:} &&
> > +                   $realfile =~ /\.c$/ &&
Do I understand this right that with this change in the check would be
applied to .c files only? .h files should be included.
> > +                   $realline == $checklicenseline)) {
What is the purpose of the above check?
> >                       if (ERROR("C99_COMMENTS",
> >                                 "do not use C99 // comments\n" . $herecurr) &&
> >                           $fix) {
>

Re: [PATCH] checkpatch: prevent reporting C99_COMMENTS error for SPDX tag in .c file

[Quentin Monnet]

On Tue, 30 Jun 2020 at 18:20, Vadim Bendebury <vbendeb@chromium.org> wrote:
>
> On Tue, Jun 30, 2020 at 7:47 AM Joe Perches <joe@perches.com> wrote:
> >
> > (adding Vadem Bendebury who added the tolerance test)
> >
> > On Tue, 2020-06-30 at 15:35 +0100, Quentin Monnet wrote:
> > > When checkpatch.pl is invoked with "--ignore C99_COMMENT_TOLERANCE", it
> > > reports C99-style comments found in the code, by matching on the
> > > double-slash pattern "//". This includes the leading slashes before the
> > > SPDX tags that are now used in a majority of C files.
> > >
> > > Such tags are commented with the double-slash on purpose, and should not
> > > trigger errors from checkpatch. Let's ignore them when searching for
> > > C99-style comments to report.
> > >
> > > Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >
> > I think this unnecessary as perhaps those that want no
> > c99 comments likely _really_ want no c99 comments.
> >
> > > ---
> > >  scripts/checkpatch.pl | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 3cacc122c528..67f350c580ea 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3983,7 +3983,10 @@ sub process {
> > >               }
> > >
> > >  # no C99 // comments
> > > -             if ($line =~ m{//}) {
> > > +             if ($line =~ m{//} &&
> > > +                 !($rawline =~ m{// SPDX-License-Identifier:} &&
> > > +                   $realfile =~ /\.c$/ &&
> Do I understand this right that with this change in the check would be
> applied to .c files only? .h files should be included.

The idea is to exclude (from the search for C99-style comments) the
SPDX tags at the top of source code files. The convention in kernel
code is that .c files use C99 comments for those tags, but .h files do
not and use "/* */" comments instead (details and motivation in
Documentation/process/license-rules.rst). So having "//" at the top of
a .h file is not conventional and should always be reported; whereas
the idea for the current patch was to ignore it for .c files.

All "//" comments not identified as an SPDX tag would still be
reported by checkpatch, whether in a .c or in a .h file.

> > > +                   $realline == $checklicenseline)) {
> What is the purpose of the above check?

This is for the "at the top" part. Because the SPDX tag is expected to
be at the top of the file (Unless I am mistaken, $checklicenseline
typically equals 1 for .c files). So with the patch, we consider a "//
SPDX-License-Identifier:" string at the top of the .c file as a SPDX
tag (and do not report a C99_COMMENTS error), but not if it is in the
middle of a file.

[...]

Thank you Joe and Vadim for your feedback. My intent is to use
checkpatch on a third-party repository where we mostly follow kernel
style (including for SPDX tags) and do not desire "//" comments. I
found it strange to have this option to report C99_COMMENTS errors
that would raise a high number of false positives on the kernel
repository (or for the repo I work with), hence the patch.

But if the feeling is that there is too little value in the change, or
if it goes against the intent of the reports for C99-style comments,
there's not much to do but to drop the patch. I can't imagine adding
new command line options for such a corner case.

Regards,
Quentin