[alsa-devel] Checkpatch warnings on switch case fallthru
Hello,
I receive a set of checkpatch warnings against my switch fallthru structure in readable_registers(). I¹m able to eliminate these warnings with a preceding comment, but that does messy the code. Is there a method better than switch for accomplishing this?
static bool cs43130_readable_register(struct device *dev, unsigned int reg) { switch (reg) { case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1: case CS43130_PWDN_CTL: case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6: case CS43130_PLL_SET_7: case CS43130_PLL_SET_8: case CS43130_PLL_SET_9: case CS43130_PLL_SET_10: case CS43130_CLKOUT_CTL: case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF: case CS43130_HP_OUT_CTL_1: case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2: case CS43130_CLASS_H_CTL: case CS43130_HP_DETECT ... CS43130_HP_STATUS: case CS43130_HP_LOAD_1: case CS43130_HP_LOAD_STAT: case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5: return true; default: return false; } }
140: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_PLL_SET_7: 141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_PLL_SET_8: 142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_PLL_SET_9: 143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_PLL_SET_10: 144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_CLKOUT_CTL: 152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_HP_OUT_CTL_1: 154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_CLASS_H_CTL: 156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_HP_LOAD_1: 160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment + case CS43130_HP_LOAD_STAT:
Thanks -Ryan
On Fri, 08 Apr 2016 00:23:03 +0200, Harvey, Ryan wrote:
Hello,
I receive a set of checkpatch warnings against my switch fallthru structure in readable_registers(). I¹m able to eliminate these warnings with a preceding comment, but that does messy the code. Is there a method better than switch for accomplishing this?
static bool cs43130_readable_register(struct device *dev, unsigned int reg) { switch (reg) { case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1: case CS43130_PWDN_CTL: case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6: case CS43130_PLL_SET_7: case CS43130_PLL_SET_8: case CS43130_PLL_SET_9: case CS43130_PLL_SET_10: case CS43130_CLKOUT_CTL: case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF: case CS43130_HP_OUT_CTL_1: case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2: case CS43130_CLASS_H_CTL: case CS43130_HP_DETECT ... CS43130_HP_STATUS: case CS43130_HP_LOAD_1: case CS43130_HP_LOAD_STAT: case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5: return true; default: return false; } }
140: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_7:
141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_8:
142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_9:
143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_10:
144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLKOUT_CTL:
152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_OUT_CTL_1:
154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLASS_H_CTL:
156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_1:
160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_STAT:
You may ignore the warnings. The purpose of checkpatch is to improve the code readability, and what it suggests is exactly opposite.
Takashi
On 04/09/2016 11:06 AM, Takashi Iwai wrote:
On Fri, 08 Apr 2016 00:23:03 +0200, Harvey, Ryan wrote:
Hello,
I receive a set of checkpatch warnings against my switch fallthru structure in readable_registers(). I¹m able to eliminate these warnings with a preceding comment, but that does messy the code. Is there a method better than switch for accomplishing this?
static bool cs43130_readable_register(struct device *dev, unsigned int reg) { switch (reg) { case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1: case CS43130_PWDN_CTL: case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6: case CS43130_PLL_SET_7: case CS43130_PLL_SET_8: case CS43130_PLL_SET_9: case CS43130_PLL_SET_10: case CS43130_CLKOUT_CTL: case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF: case CS43130_HP_OUT_CTL_1: case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2: case CS43130_CLASS_H_CTL: case CS43130_HP_DETECT ... CS43130_HP_STATUS: case CS43130_HP_LOAD_1: case CS43130_HP_LOAD_STAT: case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5: return true; default: return false; } }
140: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_7:
141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_8:
142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_9:
143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_10:
144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLKOUT_CTL:
152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_OUT_CTL_1:
154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLASS_H_CTL:
156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_1:
160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_STAT:
You may ignore the warnings. The purpose of checkpatch is to improve the code readability, and what it suggests is exactly opposite.
It's a bug in checkpatch. It seems to get confused by the "...". The best approach to solving those warnings is to create a fix for checkpatch.
On Sat, 2016-04-09 at 18:07 +0200, Lars-Peter Clausen wrote:
On 04/09/2016 11:06 AM, Takashi Iwai wrote:
On Fri, 08 Apr 2016 00:23:03 +0200, Harvey, Ryan wrote:
Hello,
I receive a set of checkpatch warnings against my switch fallthru structure in readable_registers(). I¹m able to eliminate these warnings with a preceding comment, but that does messy the code. Is there a method better than switch for accomplishing this?
static bool cs43130_readable_register(struct device *dev, unsigned int reg) { switch (reg) { case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1: case CS43130_PWDN_CTL: case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6: case CS43130_PLL_SET_7: case CS43130_PLL_SET_8: case CS43130_PLL_SET_9: case CS43130_PLL_SET_10: case CS43130_CLKOUT_CTL: case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF: case CS43130_HP_OUT_CTL_1: case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2: case CS43130_CLASS_H_CTL: case CS43130_HP_DETECT ... CS43130_HP_STATUS: case CS43130_HP_LOAD_1: case CS43130_HP_LOAD_STAT: case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5: return true; default: return false; } }
140: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_7:
141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_8:
142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_9:
143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_PLL_SET_10:
144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLKOUT_CTL:
152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_OUT_CTL_1:
154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_CLASS_H_CTL:
156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_1:
160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment
- case CS43130_HP_LOAD_STAT:
You may ignore the warnings. The purpose of checkpatch is to improve the code readability, and what it suggests is exactly opposite.
It's a bug in checkpatch. It seems to get confused by the "...". The best approach to solving those warnings is to create a fix for checkpatch.
Can you send me the checkpatch.pl file you are using please?
The current switch/case test doesn't handle ... case labels like:
switch (foo) { case bar ... baz: etc... }
Add a specific regex for that form and the default label. Use the regex where a case label is tested.
Improve the missing break/fall-through test by only reporting the first case label missing the break not every case label where there isn't a preceding break/fall-through.
Show the line above the case label when reporting the possible defect.
Signed-off-by: Joe Perches joe@perches.com Reported-by: Lars-Peter Clausen lars@metafoo.de --- scripts/checkpatch.pl | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index e3d9c34..216e4a1 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -331,6 +331,11 @@ our $Operators = qr{ }x; our $c90_Keywords = qr{do|for|while|if|else|return|goto|continue|switch|default|case|break}x; +our $case_label = qr{ + (?:case\s+(?:$Ident|$Constant) + (?:\s*...\s*\s*(?:$Ident|$Constant))? | + default)\s*: + }x; our $BasicType; our $NonptrType; @@ -4417,7 +4422,7 @@ sub process { $herecurr); } # case and default should not have general statements after them - if ($line =~ /^.\s*(?:case\s*.*|default\s*):/g && + if ($line =~ /^.\s*$case_label/g && $line !~ /\G(?: (?:\s*$;*)(?:\s*{)?(?:\s*$;*)(?:\s*\)?\s*$| \s*return\s+ @@ -5648,27 +5653,28 @@ sub process { } # check for case / default statements not preceded by break/fallthrough/switch - if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*|default):/) { + if ($line =~ /^.\s*$case_label/ && + $prevline !~ /^.\s*$case_label/) { my $has_break = 0; my $has_statement = 0; my $count = 0; my $prevline = $linenr; - while ($prevline > 1 && ($file || $count < 3) && !$has_break) { + while ($prevline > 1 && ($file || $count < 3) && !$has_break && !$has_statement) { $prevline--; my $rline = $rawlines[$prevline - 1]; my $fline = $lines[$prevline - 1]; last if ($fline =~ /^@@/); next if ($fline =~ /^-/); - next if ($fline =~ /^.(?:\s*(?:case\s+(?:$Ident|$Constant)[\s$;]*|default):[\s$;]*)*$/); + next if ($fline =~ /^.(?:\s*${case_label}[\s$;]*)*$/); $has_break = 1 if ($rline =~ /fall[\s_-]*(through|thru)/i); next if ($fline =~ /^.[\s$;]*$/); - $has_statement = 1; $count++; $has_break = 1 if ($fline =~ /\bswitch\b|\b(?:break\s*;[\s$;]*$|return\b|goto\b|continue\b)/); + $has_statement = 1; } if (!$has_break && $has_statement) { WARN("MISSING_BREAK", - "Possible switch case/default not preceeded by break or fallthrough comment\n" . $herecurr); + "Possible switch case/default not preceeded by break or fallthrough comment\n" . $hereprev); } }
On Sun, 2016-04-10 at 19:26 -0700, Joe Perches wrote:
The current switch/case test doesn't handle ... case labels like:
switch (foo) { case bar ... baz: etc... }
Add a specific regex for that form and the default label. Use the regex where a case label is tested.
Please ignore, the patch is defective as it has too many false positives.
participants (4)
-
Harvey, Ryan -
Joe Perches -
Lars-Peter Clausen -
Takashi Iwai