From: Rusty Russell <rusty@rustcorp.com.au>
To: Minfei Huang <mhuang@redhat.com>, akpm@linux-foundation.org,
        rob.jones@codethink.co.uk, amhyung@kernel.org
Cc: linux-kernel@vger.kernel.org, Minfei Huang <mnfhuang@gmail.com>
Subject: Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
In-Reply-To: <1436857153-18874-2-git-send-email-mhuang@redhat.com>
References: <1436857153-18874-1-git-send-email-mhuang@redhat.com> <1436857153-18874-2-git-send-email-mhuang@redhat.com>
User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu)
Date: Wed, 15 Jul 2015 07:22:32 +0930
Message-ID: <87twt676fz.fsf@rustcorp.com.au>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3332
Lines: 95

Minfei Huang <mhuang@redhat.com> writes:
> From: Minfei Huang <mnfhuang@gmail.com>
>
> It is not elegance, if we use function directly as the argument, like
> following:
>
> bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
>                                    struct module *owner,
>                                    void *data), void *data);
>
> Here introduce a type defined function find_symbol_in_section_t. Now
> we can use these type defined function directly, if we want to pass
> the function as the argument.
>
> bool each_symbol_section(find_symbol_in_section_t fn, void *data);

I disagree.

It's shorter, but it's less clear.  typedefs on functions are not very
useful:
1) They require readers to look in two places to see how to use the
   function (ie each_symbol_section).
2) They can't use the typedef to declare their function, since that
   doesn't work in C.

If the function were being used many times, it makes sense.  But
it's only used twice, once static inside module.c.

So I won't be applying these.

Cheers,
Rusty.

> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
>  include/linux/module.h | 6 +++---
>  kernel/module.c        | 9 ++-------
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index d67b193..1e125b1 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -462,14 +462,14 @@ const struct kernel_symbol *find_symbol(const char *name,
>  					bool gplok,
>  					bool warn);
>  
> +typedef bool (*find_symbol_in_section_t)(const struct symsearch *arr,
> +		struct module *owner, void *data);
>  /*
>   * Walk the exported symbol table
>   *
>   * Must be called with module_mutex held or preemption disabled.
>   */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> -				    struct module *owner,
> -				    void *data), void *data);
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>  
>  /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
>     symnum out of range. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 4d2b82e..1400c0b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -426,9 +426,7 @@ extern const unsigned long __start___kcrctab_unused_gpl[];
>  static bool each_symbol_in_section(const struct symsearch *arr,
>  				   unsigned int arrsize,
>  				   struct module *owner,
> -				   bool (*fn)(const struct symsearch *syms,
> -					      struct module *owner,
> -					      void *data),
> +				   find_symbol_in_section_t fn,
>  				   void *data)
>  {
>  	unsigned int j;
> @@ -442,10 +440,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
>  }
>  
>  /* Returns true as soon as fn returns true, otherwise false. */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> -				    struct module *owner,
> -				    void *data),
> -			 void *data)
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data)
>  {
>  	struct module *mod;
>  	static const struct symsearch arr[] = {
> -- 
> 2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/