* [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues @ 2020-09-28 15:47 Dwaipayan Ray 2020-09-28 16:14 ` Lukas Bulwahn 0 siblings, 1 reply; 4+ messages in thread From: Dwaipayan Ray @ 2020-09-28 15:47 UTC (permalink / raw) To: Lukas Bulwahn, linux-kernel-mentees Hi, Checkpatch seems to generate some false positives on certain macros. For example running checkpatch on (kernel/trace/trace_export.c), ERROR: Macros with multiple statements should be enclosed in a do - while loop #49: FILE: kernel/trace/trace_export.c:49: +#define __field_packed(type, container, item) type item; ERROR: Macros with multiple statements should be enclosed in a do - while loop #52: FILE: kernel/trace/trace_export.c:52: +#define __array(type, item, size) type item[size]; and several other same errors. Wrapping this in a do - while certainly doesn't make sense. Removing the semicolon at the end of macro and appending semicolon in the call changes the error to: ERROR: Macros with complex values should be enclosed in parentheses #49: FILE: kernel/trace/trace_export.c:49: +#define __array_desc(type, container, item, size) type item[size] This seems more reasonable. But the error isn't necessary in the first place. Thanks, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues 2020-09-28 15:47 [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues Dwaipayan Ray @ 2020-09-28 16:14 ` Lukas Bulwahn 2020-09-28 17:11 ` Dwaipayan Ray 0 siblings, 1 reply; 4+ messages in thread From: Lukas Bulwahn @ 2020-09-28 16:14 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees On Mon, 28 Sep 2020, Dwaipayan Ray wrote: > Hi, > Checkpatch seems to generate some false positives on > certain macros. > > For example running checkpatch on (kernel/trace/trace_export.c), > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > #49: FILE: kernel/trace/trace_export.c:49: > +#define __field_packed(type, container, item) type item; > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > #52: FILE: kernel/trace/trace_export.c:52: > +#define __array(type, item, size) type item[size]; > I guess checkpatch.pl detects the semicolon and thinks it must be two statements. How about checking if the semicolon is actually followed by anything or not? > and several other same errors. > Are they all in one file or spread around in the kernel everywhere? > Wrapping this in a do - while certainly doesn't make sense. > Removing the semicolon at the end of macro and appending semicolon > in the call changes the error to: > > ERROR: Macros with complex values should be enclosed in parentheses > #49: FILE: kernel/trace/trace_export.c:49: > +#define __array_desc(type, container, item, size) type item[size] > > This seems more reasonable. But the error isn't necessary in the first > place. > Considering format or semantic checks, I would always check if the better check can be implemented with clang-format, clang-analyzer or coccinelle. checkpatch.pl is always just a quick heuristics; if other checking tools become part of standard check, then we could actually just refer to those checking tools instead. Lukas > Thanks, > Dwaipayan. > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues 2020-09-28 16:14 ` Lukas Bulwahn @ 2020-09-28 17:11 ` Dwaipayan Ray 2020-09-29 7:58 ` Lukas Bulwahn 0 siblings, 1 reply; 4+ messages in thread From: Dwaipayan Ray @ 2020-09-28 17:11 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: linux-kernel-mentees On Mon, Sep 28, 2020 at 9:44 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > > > > On Mon, 28 Sep 2020, Dwaipayan Ray wrote: > > > Hi, > > Checkpatch seems to generate some false positives on > > certain macros. > > > > For example running checkpatch on (kernel/trace/trace_export.c), > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > > #49: FILE: kernel/trace/trace_export.c:49: > > +#define __field_packed(type, container, item) type item; > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > > #52: FILE: kernel/trace/trace_export.c:52: > > +#define __array(type, item, size) type item[size]; > > > > I guess checkpatch.pl detects the semicolon and thinks it must be two > statements. How about checking if the semicolon is actually followed by > anything or not? > > > and several other same errors. > > > > Are they all in one file or spread around in the kernel everywhere? > Yes, there were all in the same file. I have to run checkpatch on other files extensively before I can confirm these issues exist elsewhere. But yes, checking for a semicolon followed by some characters does solve the problem, and changes the warning to a (Macros with complex values...). I do agree that such complex checks should be left to clang-format or likewise. Not sure if it is worthwhile merging, but here goes the diff: --- diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 9e65d21456f1..8382977e95fd 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5351,7 +5367,7 @@ sub process { if ($dstat =~ /^\s*if\b/) { ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", "Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects\n" . "$herectx"); - } elsif ($dstat =~ /;/) { + } elsif ($dstat =~ /;.+/) { ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", "Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx"); } else { --- Thanks, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues 2020-09-28 17:11 ` Dwaipayan Ray @ 2020-09-29 7:58 ` Lukas Bulwahn 0 siblings, 0 replies; 4+ messages in thread From: Lukas Bulwahn @ 2020-09-29 7:58 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees On Mon, 28 Sep 2020, Dwaipayan Ray wrote: > On Mon, Sep 28, 2020 at 9:44 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote: > > > > > > > > On Mon, 28 Sep 2020, Dwaipayan Ray wrote: > > > > > Hi, > > > Checkpatch seems to generate some false positives on > > > certain macros. > > > > > > For example running checkpatch on (kernel/trace/trace_export.c), > > > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > > > #49: FILE: kernel/trace/trace_export.c:49: > > > +#define __field_packed(type, container, item) type item; > > > > > > ERROR: Macros with multiple statements should be enclosed in a do - while loop > > > #52: FILE: kernel/trace/trace_export.c:52: > > > +#define __array(type, item, size) type item[size]; > > > > > > > I guess checkpatch.pl detects the semicolon and thinks it must be two > > statements. How about checking if the semicolon is actually followed by > > anything or not? > > > > > and several other same errors. > > > > > > > Are they all in one file or spread around in the kernel everywhere? > > > > Yes, there were all in the same file. I have to run checkpatch on other > files extensively before I can confirm these issues exist elsewhere. > Okay, maybe I can support here with some local computing power from my side. > But yes, checking for a semicolon followed by some characters > does solve the problem, and changes the warning to a (Macros > with complex values...). > Yes, unfortunately, macros with complex values is still wrong here... > I do agree that such complex checks should be left to clang-format or > likewise. > > Not sure if it is worthwhile merging, but here goes the diff: > > --- > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 9e65d21456f1..8382977e95fd 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > > @@ -5351,7 +5367,7 @@ sub process { > if ($dstat =~ /^\s*if\b/) { > > ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", > "Macros starting with if > should be enclosed by a do - while loop to avoid possible if/else > logic defects\n" . "$herectx"); > - } elsif ($dstat =~ /;/) { > + } elsif ($dstat =~ /;.+/) { > > ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", > "Macros with multiple > statements should be enclosed in a do - while loop\n" . "$herectx"); > } else { > --- How about sending a first proper commit at least here for the mentees? I guess if we can run the checkpatch.pl evaluation on that commit, we can see and argue if that is worth the inclusion. I get that it solves the issues above, but it is tricky to say if something unrelated now is much worse... A proper evaluation will show. Lukas > Thanks, > Dwaipayan. > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-29 7:59 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-28 15:47 [Linux-kernel-mentees] checkpatch.pl investigation: MULTISTATEMENT_MACRO_USE_DO_WHILE issues Dwaipayan Ray 2020-09-28 16:14 ` Lukas Bulwahn 2020-09-28 17:11 ` Dwaipayan Ray 2020-09-29 7:58 ` Lukas Bulwahn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).