All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Joe Perches <joe@perches.com>, Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sound: use enum names instead of magic numbers
Date: Sun, 30 Nov 2014 13:52:02 +0900	[thread overview]
Message-ID: <547AA272.6070308@sakamocchi.jp> (raw)
In-Reply-To: <1417294692.818.3.camel@perches.com>

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

WARNING: multiple messages have this Message-ID (diff)
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Joe Perches <joe@perches.com>, Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sound: use enum names instead of magic numbers
Date: Sun, 30 Nov 2014 13:52:02 +0900	[thread overview]
Message-ID: <547AA272.6070308@sakamocchi.jp> (raw)
In-Reply-To: <1417294692.818.3.camel@perches.com>

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


  reply	other threads:[~2014-11-30  4:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-28 18:59 [PATCH] Sound: drivers: virmidi: fixed code style issues Kyle Chamberlin
2014-11-29 20:36 ` Takashi Iwai
2014-11-29 20:36   ` Takashi Iwai
2014-11-29 20:58   ` [PATCH] sound: use enum names instead of magic numbers Joe Perches
2014-11-30  4:52     ` Takashi Sakamoto [this message]
2014-11-30  4:52       ` Takashi Sakamoto
2014-11-30  4:57       ` Joe Perches
2014-11-30  8:41     ` Takashi Iwai
2014-11-30  8:55       ` Joe Perches
2014-11-30  9:04         ` Takashi Iwai
2014-11-30 19:26           ` Joe Perches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=547AA272.6070308@sakamocchi.jp \
    --to=o-takashi@sakamocchi.jp \
    --cc=alsa-devel@alsa-project.org \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.