Skip to contentSkip to conversation

Checkpatch warnings on switch case fallthru

PATCH In review Harvey, Ryan 4 participants 6 messages Apr 7, 2016 – Apr 11, 2016 Checkpatch

lore
Share & tools
lore

Harvey, Ryan

author
#0 lore raw

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

#1 lore raw
Quoted 60 lines · Ryan

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

(quoted diff) +1 -0
+	case CS43130_PLL_SET_7:

141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_8:

142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_9:

143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_10:

144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLKOUT_CTL:

152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_OUT_CTL_1:

154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLASS_H_CTL:

156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_LOAD_1:

160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

Review

Quoted 1 line
(quoted diff) +1 -0
+	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
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#2 lore raw
Quoted 58 lines · Takashi Iwai
Quoted 56 lines · Ryan

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

(quoted diff) +1 -0
+	case CS43130_PLL_SET_7:

141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_8:

142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_9:

143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_10:

144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLKOUT_CTL:

152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_OUT_CTL_1:

154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLASS_H_CTL:

156: [WARNING] Possible switch case/default not preceeded by break or

Quoted 8 lines

Review

Quoted 5 lines

fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_LOAD_1:

160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	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.

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

maintainer
#3 lore raw
Quoted 68 lines · Lars-Peter Clausen
Quoted 59 lines · Takashi Iwai
Quoted 56 lines · Ryan

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

(quoted diff) +1 -0
+	case CS43130_PLL_SET_7:

141: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_8:

142: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_9:

143: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_PLL_SET_10:

144: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLKOUT_CTL:

152: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_OUT_CTL_1:

154: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_CLASS_H_CTL:
Quoted 8 lines

Review

Quoted 6 lines

156: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	case CS43130_HP_LOAD_1:

160: [WARNING] Possible switch case/default not preceeded by break or fallthrough comment

(quoted diff) +1 -0
+	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.

Quoted 2 lines

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?

maintainer
#4 lore raw

PATCH 1/1 checkpatch: Improve missing break for switch/case tests

In reply to Lars-Peter Clausen

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 <[email protected]>
Reported-by Lars-Peter Clausen <[email protected]>

---
 scripts/checkpatch.pl | 18 ++++++++++++------

 1 file changed, 12 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +0 -0
index e3d9c34..216e4a1 100755
@@ -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);
 			}
 		}
 
maintainer
#5 lore raw

Re: [PATCH] checkpatch: Improve missing break for switch/case tests

Quoted 9 lines · Joe Perches

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.