Re: [alsa-devel] Fwd: [PATCH 7/9] ASoC: Blackfin: I2S CPU DAI driver

On Thu, Oct 9, 2008 at 9:35 AM, Valdis.Kletnieks@vt.edu wrote:

On Thu, 09 Oct 2008 08:58:08 +0200, John Kacur said:

...

My eyes fell upon this switch statement, probably I have similar criticisms as to what has already been said, but:

1. Surely the default case is also an -EINVAL
2. Why not let all the EINVALS fall through, it will shorten up the

code, and IMO make it more readable, something like this?

•   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
  

•   case SND_SOC_DAIFMT_CBM_CFM: /* Passing Case */
  

•           break;
  

•   case SND_SOC_DAIFMT_CBS_CFS: /* Failing Cases */
  

•   case SND_SOC_DAIFMT_CBM_CFS:
  

•   case SND_SOC_DAIFMT_CBS_CFM:
  

•           ret = -EINVAL;
  

•           break;
  

•   default:
  

•           printk(KERN_INFO "Unknown SND_SOC_DAIFMT kind\n");
  

•           ret = -EINVAL;
  

•           break;
  

•   }
  

Even shorter, but puts the default in a non-standard place:

•   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
  

•   case SND_SOC_DAIFMT_CBM_CFM: /* Passing Case */
  

•           break;
  

•   default:                        /* So much Fail we should say something */
  

•           printk(KERN_INFO "Unknown SND_SOC_DAIFMT kind\n");
  

•   case SND_SOC_DAIFMT_CBS_CFS: /* Failing Cases */
  

•   case SND_SOC_DAIFMT_CBM_CFS:
  

•   case SND_SOC_DAIFMT_CBS_CFM:
  

•           ret = -EINVAL;
  

•           break;
  

•   }
  
  

(I see a checkpatch update to check where default: is, coming in 3.. 2.. 1.. :)

Well, of course you only want to make things shorter when it increases clarity, you don't want to make things shorter for the sake of making them shorter, so yeah, I would nix that non-standard default position too.