sound: use enum names instead of magic numbers
diff mbox

Message ID 1417294692.818.3.camel@perches.com
State New, archived
Headers show

Commit Message

Joe Perches Nov. 29, 2014, 8:58 p.m. UTC
There's an enum defined for these magic numbers,
might as well use it.

Miscellanea:
o Use ##__VA_ARGS__

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/sound/core.h | 28 +++++++++++++++-------------
 sound/core/misc.c    |  6 +++---
 2 files changed, 18 insertions(+), 16 deletions(-)

Comments

Takashi Sakamoto Nov. 30, 2014, 4:52 a.m. UTC | #1
On 2014?11?30? 05:58, Joe Perches wrote:
> There's an enum defined for these magic numbers,
> might as well use it.
> 
> Miscellanea:
> o Use ##__VA_ARGS__
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  include/sound/core.h | 28 +++++++++++++++-------------
>  sound/core/misc.c    |  6 +++---
>  2 files changed, 18 insertions(+), 16 deletions(-)

I think it better to split this patch into two parts, one is for
enumeration identifier and another is for variadic macro.

> diff --git a/include/sound/core.h b/include/sound/core.h
> index 1df3f2f..40418e7 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -325,7 +325,7 @@ void release_and_free_resource(struct resource *res);
>  /* --- */
>  
>  /* sound printk debug levels */
> -enum {
> +enum snd_level {
>  	SND_PR_ALWAYS,
>  	SND_PR_DEBUG,
>  	SND_PR_VERBOSE,
> @@ -333,11 +333,11 @@ enum {
>  
>  #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
>  __printf(4, 5)
> -void __snd_printk(unsigned int level, const char *file, int line,
> +void __snd_printk(enum snd_level snd_level, const char *file, int line,
>  		  const char *format, ...);
>  #else
> -#define __snd_printk(level, file, line, format, args...) \
> -	printk(format, ##args)
> +#define __snd_printk(snd_level, file, line, format, ...) \
> +	printk(format, ##__VA_ARGS__)
>  #endif
>  /**
> @@ -347,8 +347,8 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * Works like printk() but prints the file and the line of the caller
>   * when configured with CONFIG_SND_VERBOSE_PRINTK.
>   */
> -#define snd_printk(fmt, args...) \
> -	__snd_printk(0, __FILE__, __LINE__, fmt, ##args)
> +#define snd_printk(fmt, ...) \
> +	__snd_printk(SND_PR_ALWAYS, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>  
>  #ifdef CONFIG_SND_DEBUG
>  /**
> @@ -358,10 +358,10 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * Works like snd_printk() for debugging purposes.
>   * Ignored when CONFIG_SND_DEBUG is not set.
>   */
> -#define snd_printd(fmt, args...) \
> -	__snd_printk(1, __FILE__, __LINE__, fmt, ##args)
> -#define _snd_printd(level, fmt, args...) \
> -	__snd_printk(level, __FILE__, __LINE__, fmt, ##args)
> +#define snd_printd(fmt, ...) \
> +	__snd_printk(SND_PR_DEBUG, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> +#define _snd_printd(level, fmt, ...) \
> +	__snd_printk(level, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>  
>  /**
>   * snd_BUG - give a BUG warning message and stack trace
> @@ -390,7 +390,8 @@ void __snd_printk(unsigned int level, const char *file, int line,
>  __printf(1, 2)
>  static inline void snd_printd(const char *format, ...) {}
>  __printf(2, 3)
> -static inline void _snd_printd(int level, const char *format, ...) {}
> +static inline void _snd_printd(enum snd_level snd_level,
> +			       const char *format, ...) {}
>  
>  #define snd_BUG()			do { } while (0)
>  
> @@ -411,8 +412,9 @@ static inline bool snd_printd_ratelimit(void) { return false; }
>   * Works like snd_printk() for debugging purposes.
>   * Ignored when CONFIG_SND_DEBUG_VERBOSE is not set.
>   */
> -#define snd_printdd(format, args...) \
> -	__snd_printk(2, __FILE__, __LINE__, format, ##args)
> +#define snd_printdd(format, ...)				\
> +	__snd_printk(SND_PR_VERBOSE, __FILE__, __LINE__,	\
> +		     format, ##__VA_ARGS__)
>  #else
>  __printf(1, 2)
>  static inline void snd_printdd(const char *format, ...) {}
> diff --git a/sound/core/misc.c b/sound/core/misc.c
> index f2e8226..03b3f56 100644
> --- a/sound/core/misc.c
> +++ b/sound/core/misc.c
> @@ -63,7 +63,7 @@ static const char *sanity_file_name(const char *path)
>  #endif
>  
>  #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
> -void __snd_printk(unsigned int level, const char *path, int line,
> +void __snd_printk(enum snd_level snd_level, const char *path, int line,
>  		  const char *format, ...)
>  {
>  	va_list args;
> @@ -74,7 +74,7 @@ void __snd_printk(unsigned int level, const char *path, int line,
>  #endif
>  
>  #ifdef CONFIG_SND_DEBUG
> -	if (debug < level)
> +	if (debug < snd_level)
>  		return;
>  #endif
>  
> @@ -88,7 +88,7 @@ void __snd_printk(unsigned int level, const char *path, int line,
>  		const char *end_of_header = printk_skip_level(format);
>  		memcpy(verbose_fmt, format, end_of_header - format);
>  		vaf.fmt = end_of_header;
> -	} else if (level)
> +	} else if (snd_level)
>  		memcpy(verbose_fmt, KERN_DEBUG, sizeof(KERN_DEBUG) - 1);
>  	printk(verbose_fmt, sanity_file_name(path), line, &vaf);

There's no need to rename variable names.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
Joe Perches Nov. 30, 2014, 4:57 a.m. UTC | #2
On Sun, 2014-11-30 at 13:52 +0900, Takashi Sakamoto wrote:
> On 2014?11?30? 05:58, Joe Perches wrote:
> > There's an enum defined for these magic numbers,
> > might as well use it.
[]
> I think it better to split this patch into two parts, one is for
> enumeration identifier and another is for variadic macro.

Go ahead then.
Takashi Iwai Nov. 30, 2014, 8:41 a.m. UTC | #3
At Sat, 29 Nov 2014 12:58:12 -0800,
Joe Perches wrote:
> 
> There's an enum defined for these magic numbers,
> might as well use it.
> 
> Miscellanea:
> o Use ##__VA_ARGS__
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Any specific reason to hang to an irrelevant thread?

Also...

> ---
>  include/sound/core.h | 28 +++++++++++++++-------------
>  sound/core/misc.c    |  6 +++---
>  2 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 1df3f2f..40418e7 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -325,7 +325,7 @@ void release_and_free_resource(struct resource *res);
>  /* --- */
>  
>  /* sound printk debug levels */
> -enum {
> +enum snd_level {
>  	SND_PR_ALWAYS,
>  	SND_PR_DEBUG,
>  	SND_PR_VERBOSE,
> @@ -333,11 +333,11 @@ enum {
>  
>  #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
>  __printf(4, 5)
> -void __snd_printk(unsigned int level, const char *file, int line,
> +void __snd_printk(enum snd_level snd_level, const char *file, int line,
>  		  const char *format, ...);
>  #else
> -#define __snd_printk(level, file, line, format, args...) \
> -	printk(format, ##args)
> +#define __snd_printk(snd_level, file, line, format, ...) \
> +	printk(format, ##__VA_ARGS__)

It's better to keep the argument name "level".  Using both the same
name for a variable and its enum type is rather confusing, and just
gives unnecessary LOCs.


thanks,

Takashi

>  #endif
>  
>  /**
> @@ -347,8 +347,8 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * Works like printk() but prints the file and the line of the caller
>   * when configured with CONFIG_SND_VERBOSE_PRINTK.
>   */
> -#define snd_printk(fmt, args...) \
> -	__snd_printk(0, __FILE__, __LINE__, fmt, ##args)
> +#define snd_printk(fmt, ...) \
> +	__snd_printk(SND_PR_ALWAYS, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>  
>  #ifdef CONFIG_SND_DEBUG
>  /**
> @@ -358,10 +358,10 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * Works like snd_printk() for debugging purposes.
>   * Ignored when CONFIG_SND_DEBUG is not set.
>   */
> -#define snd_printd(fmt, args...) \
> -	__snd_printk(1, __FILE__, __LINE__, fmt, ##args)
> -#define _snd_printd(level, fmt, args...) \
> -	__snd_printk(level, __FILE__, __LINE__, fmt, ##args)
> +#define snd_printd(fmt, ...) \
> +	__snd_printk(SND_PR_DEBUG, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> +#define _snd_printd(level, fmt, ...) \
> +	__snd_printk(level, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>  
>  /**
>   * snd_BUG - give a BUG warning message and stack trace
> @@ -390,7 +390,8 @@ void __snd_printk(unsigned int level, const char *file, int line,
>  __printf(1, 2)
>  static inline void snd_printd(const char *format, ...) {}
>  __printf(2, 3)
> -static inline void _snd_printd(int level, const char *format, ...) {}
> +static inline void _snd_printd(enum snd_level snd_level,
> +			       const char *format, ...) {}
>  
>  #define snd_BUG()			do { } while (0)
>  
> @@ -411,8 +412,9 @@ static inline bool snd_printd_ratelimit(void) { return false; }
>   * Works like snd_printk() for debugging purposes.
>   * Ignored when CONFIG_SND_DEBUG_VERBOSE is not set.
>   */
> -#define snd_printdd(format, args...) \
> -	__snd_printk(2, __FILE__, __LINE__, format, ##args)
> +#define snd_printdd(format, ...)				\
> +	__snd_printk(SND_PR_VERBOSE, __FILE__, __LINE__,	\
> +		     format, ##__VA_ARGS__)
>  #else
>  __printf(1, 2)
>  static inline void snd_printdd(const char *format, ...) {}
> diff --git a/sound/core/misc.c b/sound/core/misc.c
> index f2e8226..03b3f56 100644
> --- a/sound/core/misc.c
> +++ b/sound/core/misc.c
> @@ -63,7 +63,7 @@ static const char *sanity_file_name(const char *path)
>  #endif
>  
>  #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
> -void __snd_printk(unsigned int level, const char *path, int line,
> +void __snd_printk(enum snd_level snd_level, const char *path, int line,
>  		  const char *format, ...)
>  {
>  	va_list args;
> @@ -74,7 +74,7 @@ void __snd_printk(unsigned int level, const char *path, int line,
>  #endif
>  
>  #ifdef CONFIG_SND_DEBUG
> -	if (debug < level)
> +	if (debug < snd_level)
>  		return;
>  #endif
>  
> @@ -88,7 +88,7 @@ void __snd_printk(unsigned int level, const char *path, int line,
>  		const char *end_of_header = printk_skip_level(format);
>  		memcpy(verbose_fmt, format, end_of_header - format);
>  		vaf.fmt = end_of_header;
> -	} else if (level)
> +	} else if (snd_level)
>  		memcpy(verbose_fmt, KERN_DEBUG, sizeof(KERN_DEBUG) - 1);
>  	printk(verbose_fmt, sanity_file_name(path), line, &vaf);
>  
> 
>
Joe Perches Nov. 30, 2014, 8:55 a.m. UTC | #4
On Sun, 2014-11-30 at 09:41 +0100, Takashi Iwai wrote:
> At Sat, 29 Nov 2014 12:58:12 -0800,
> Joe Perches wrote:
> > 
> > There's an enum defined for these magic numbers,
> > might as well use it.
> > 
> > Miscellanea:
> > o Use ##__VA_ARGS__
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> Any specific reason to hang to an irrelevant thread?

Not in particular, I just noticed it there.

> Also...

> > diff --git a/include/sound/core.h b/include/sound/core.h
[]
> > +#define __snd_printk(snd_level, file, line, format, ...) \
> > +	printk(format, ##__VA_ARGS__)
> 
> It's better to keep the argument name "level".  Using both the same
> name for a variable and its enum type is rather confusing, and just
> gives unnecessary LOCs.

Write it as you chose.  It is pretty trivial.
Takashi Iwai Nov. 30, 2014, 9:04 a.m. UTC | #5
At Sun, 30 Nov 2014 00:55:32 -0800,
Joe Perches wrote:
> 
> On Sun, 2014-11-30 at 09:41 +0100, Takashi Iwai wrote:
> > At Sat, 29 Nov 2014 12:58:12 -0800,
> > Joe Perches wrote:
> > > 
> > > There's an enum defined for these magic numbers,
> > > might as well use it.
> > > 
> > > Miscellanea:
> > > o Use ##__VA_ARGS__
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > 
> > Any specific reason to hang to an irrelevant thread?
> 
> Not in particular, I just noticed it there.
> 
> > Also...
> 
> > > diff --git a/include/sound/core.h b/include/sound/core.h
> []
> > > +#define __snd_printk(snd_level, file, line, format, ...) \
> > > +	printk(format, ##__VA_ARGS__)
> > 
> > It's better to keep the argument name "level".  Using both the same
> > name for a variable and its enum type is rather confusing, and just
> > gives unnecessary LOCs.
> 
> Write it as you chose.  It is pretty trivial.

... trivial for anyone ;)


Takashi
Joe Perches Nov. 30, 2014, 7:26 p.m. UTC | #6
On Sun, 2014-11-30 at 10:04 +0100, Takashi Iwai wrote:
> At Sun, 30 Nov 2014 00:55:32 -0800, Joe Perches wrote:
> > On Sun, 2014-11-30 at 09:41 +0100, Takashi Iwai wrote:
> > > At Sat, 29 Nov 2014 12:58:12 -0800, Joe Perches wrote:
> > > > There's an enum defined for these magic numbers,
> > > > might as well use it.
> > > > 
> > > > Miscellanea:
> > > > o Use ##__VA_ARGS__
[]
> > Write it as you chose.  It is pretty trivial.
> 
> ... trivial for anyone ;)

The other option is simply to remove the enum
as the values are unused.

Patch
diff mbox

diff --git a/include/sound/core.h b/include/sound/core.h
index 1df3f2f..40418e7 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -325,7 +325,7 @@  void release_and_free_resource(struct resource *res);
 /* --- */
 
 /* sound printk debug levels */
-enum {
+enum snd_level {
 	SND_PR_ALWAYS,
 	SND_PR_DEBUG,
 	SND_PR_VERBOSE,
@@ -333,11 +333,11 @@  enum {
 
 #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
 __printf(4, 5)
-void __snd_printk(unsigned int level, const char *file, int line,
+void __snd_printk(enum snd_level snd_level, const char *file, int line,
 		  const char *format, ...);
 #else
-#define __snd_printk(level, file, line, format, args...) \
-	printk(format, ##args)
+#define __snd_printk(snd_level, file, line, format, ...) \
+	printk(format, ##__VA_ARGS__)
 #endif
 
 /**
@@ -347,8 +347,8 @@  void __snd_printk(unsigned int level, const char *file, int line,
  * Works like printk() but prints the file and the line of the caller
  * when configured with CONFIG_SND_VERBOSE_PRINTK.
  */
-#define snd_printk(fmt, args...) \
-	__snd_printk(0, __FILE__, __LINE__, fmt, ##args)
+#define snd_printk(fmt, ...) \
+	__snd_printk(SND_PR_ALWAYS, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
 
 #ifdef CONFIG_SND_DEBUG
 /**
@@ -358,10 +358,10 @@  void __snd_printk(unsigned int level, const char *file, int line,
  * Works like snd_printk() for debugging purposes.
  * Ignored when CONFIG_SND_DEBUG is not set.
  */
-#define snd_printd(fmt, args...) \
-	__snd_printk(1, __FILE__, __LINE__, fmt, ##args)
-#define _snd_printd(level, fmt, args...) \
-	__snd_printk(level, __FILE__, __LINE__, fmt, ##args)
+#define snd_printd(fmt, ...) \
+	__snd_printk(SND_PR_DEBUG, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
+#define _snd_printd(level, fmt, ...) \
+	__snd_printk(level, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
 
 /**
  * snd_BUG - give a BUG warning message and stack trace
@@ -390,7 +390,8 @@  void __snd_printk(unsigned int level, const char *file, int line,
 __printf(1, 2)
 static inline void snd_printd(const char *format, ...) {}
 __printf(2, 3)
-static inline void _snd_printd(int level, const char *format, ...) {}
+static inline void _snd_printd(enum snd_level snd_level,
+			       const char *format, ...) {}
 
 #define snd_BUG()			do { } while (0)
 
@@ -411,8 +412,9 @@  static inline bool snd_printd_ratelimit(void) { return false; }
  * Works like snd_printk() for debugging purposes.
  * Ignored when CONFIG_SND_DEBUG_VERBOSE is not set.
  */
-#define snd_printdd(format, args...) \
-	__snd_printk(2, __FILE__, __LINE__, format, ##args)
+#define snd_printdd(format, ...)				\
+	__snd_printk(SND_PR_VERBOSE, __FILE__, __LINE__,	\
+		     format, ##__VA_ARGS__)
 #else
 __printf(1, 2)
 static inline void snd_printdd(const char *format, ...) {}
diff --git a/sound/core/misc.c b/sound/core/misc.c
index f2e8226..03b3f56 100644
--- a/sound/core/misc.c
+++ b/sound/core/misc.c
@@ -63,7 +63,7 @@  static const char *sanity_file_name(const char *path)
 #endif
 
 #if defined(CONFIG_SND_DEBUG) || defined(CONFIG_SND_VERBOSE_PRINTK)
-void __snd_printk(unsigned int level, const char *path, int line,
+void __snd_printk(enum snd_level snd_level, const char *path, int line,
 		  const char *format, ...)
 {
 	va_list args;
@@ -74,7 +74,7 @@  void __snd_printk(unsigned int level, const char *path, int line,
 #endif
 
 #ifdef CONFIG_SND_DEBUG
-	if (debug < level)
+	if (debug < snd_level)
 		return;
 #endif
 
@@ -88,7 +88,7 @@  void __snd_printk(unsigned int level, const char *path, int line,
 		const char *end_of_header = printk_skip_level(format);
 		memcpy(verbose_fmt, format, end_of_header - format);
 		vaf.fmt = end_of_header;
-	} else if (level)
+	} else if (snd_level)
 		memcpy(verbose_fmt, KERN_DEBUG, sizeof(KERN_DEBUG) - 1);
 	printk(verbose_fmt, sanity_file_name(path), line, &vaf);