[RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
To define or to inline that is the question:
The current definition of flat_set_persistent produces a compiler 
warning; arch/sh/ does it in a different way defining it to 
a macro that uses persistent var. IMHO, an inline is easier to read.
---
 arch/m68k/include/asm/flat.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/m68k/include/asm/flat.h b/arch/m68k/include/asm/flat.h
index a0e2907..f9454b8 100644
--- a/arch/m68k/include/asm/flat.h
+++ b/arch/m68k/include/asm/flat.h
@@ -11,6 +11,11 @@
 #define	flat_get_addr_from_rp(rp, relval, flags, p)	get_unaligned(rp)
 #define	flat_put_addr_at_rp(rp, val, relval)	put_unaligned(val,rp)
 #define	flat_get_relocate_addr(rel)		(rel)
-#define	flat_set_persistent(relval, p)		0
+
+static inline int flat_set_persistent(unsigned long relval,
+				      unsigned long *persistent)
+{
+	return 0;
+}
 
 #endif /* __M68KNOMMU_FLAT_H__ */
-- 
1.7.3.4

Re: [RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Geert Uytterhoeven]

On Mon, Apr 23, 2012 at 15:50, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> To define or to inline that is the question:
> The current definition of flat_set_persistent produces a compiler
> warning; arch/sh/ does it in a different way defining it to
> a macro that uses persistent var. IMHO, an inline is easier to read.

What's the compiler warning?
It seems several other nommu arches use the same definition for
flat_set_persistent()?

> ---
>  arch/m68k/include/asm/flat.h |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/arch/m68k/include/asm/flat.h b/arch/m68k/include/asm/flat.h
> index a0e2907..f9454b8 100644
> --- a/arch/m68k/include/asm/flat.h
> +++ b/arch/m68k/include/asm/flat.h
> @@ -11,6 +11,11 @@
>  #define        flat_get_addr_from_rp(rp, relval, flags, p)     get_unaligned(rp)
>  #define        flat_put_addr_at_rp(rp, val, relval)    put_unaligned(val,rp)
>  #define        flat_get_relocate_addr(rel)             (rel)
> -#define        flat_set_persistent(relval, p)          0
> +
> +static inline int flat_set_persistent(unsigned long relval,
> +                                     unsigned long *persistent)
> +{
> +       return 0;
> +}
>
>  #endif /* __M68KNOMMU_FLAT_H__ */
> --
> 1.7.3.4

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Ezequiel García]

Hi Geert,

On Mon, Apr 23, 2012 at 6:07 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Apr 23, 2012 at 15:50, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
>> ---
>> To define or to inline that is the question:
>> The current definition of flat_set_persistent produces a compiler
>> warning; arch/sh/ does it in a different way defining it to
>> a macro that uses persistent var. IMHO, an inline is easier to read.
>
> What's the compiler warning?
> It seems several other nommu arches use the same definition for
> flat_set_persistent()?
>

Here's the warning:
  fs/binfmt_flat.c: In function ‘load_flat_file’:
  fs/binfmt_flat.c:752: warning: unused variable ‘persistent’

Yes, every arch except sh is using the same definition.
My first thought was to extend the arch/sh definition:

  #define flat_set_persistent(relval, p)          ({ (void)p; 0; })

but in a conversation in the janitors list I was told that inlining
to a return-only function is a common pattern.
Plus, if you compare fs/built-in.o there is no extra code generated:

$ size fs-built-in-flat-inline
   text	   data	    bss	    dec	    hex	filename
 233332	   1908	   1640	 236880	  39d50	built-in-flat-inline
$ size fs/built-in.o
   text	   data	    bss	    dec	    hex	filename
 233332	   1908	   1640	 236880	  39d50	fs/built-in.o

FWIW, this is my compiler, I built it using gentoo's crossdev tool.
  gcc version 4.4.5 (Gentoo 4.4.5 p1.3, pie-0.4.5)

So, what's your opinion?
Thanks,
Ezequiel.

Re: [RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Greg Ungerer]

Hi Ezequiel,

On 24/04/12 08:43, Ezequiel García wrote:
> On Mon, Apr 23, 2012 at 6:07 PM, Geert Uytterhoeven
> <geert@linux-m68k.org>  wrote:
>> On Mon, Apr 23, 2012 at 15:50, Ezequiel Garcia<elezegarcia@gmail.com>  wrote:
>>> Signed-off-by: Ezequiel Garcia<elezegarcia@gmail.com>

Normally this signed-off-by goes after your patch description
(Documentation/SubmittingPatches section 12).


>>> ---
>>> To define or to inline that is the question:
>>> The current definition of flat_set_persistent produces a compiler
>>> warning; arch/sh/ does it in a different way defining it to
>>> a macro that uses persistent var. IMHO, an inline is easier to read.
>>
>> What's the compiler warning?
>> It seems several other nommu arches use the same definition for
>> flat_set_persistent()?
>>
>
> Here's the warning:
>    fs/binfmt_flat.c: In function æload_flat_fileÆ:
>    fs/binfmt_flat.c:752: warning: unused variable æpersistentÆ

I like to see the actual compiler messages in the git message
description as well.


> Yes, every arch except sh is using the same definition.
> My first thought was to extend the arch/sh definition:
>
>    #define flat_set_persistent(relval, p)          ({ (void)p; 0; })
>
> but in a conversation in the janitors list I was told that inlining
> to a return-only function is a common pattern.
> Plus, if you compare fs/built-in.o there is no extra code generated:
>
> $ size fs-built-in-flat-inline
>     text	   data	    bss	    dec	    hex	filename
>   233332	   1908	   1640	 236880	  39d50	built-in-flat-inline
> $ size fs/built-in.o
>     text	   data	    bss	    dec	    hex	filename
>   233332	   1908	   1640	 236880	  39d50	fs/built-in.o
>
> FWIW, this is my compiler, I built it using gentoo's crossdev tool.
>    gcc version 4.4.5 (Gentoo 4.4.5 p1.3, pie-0.4.5)
>
> So, what's your opinion?

I see the same warning on my gcc-4.5.1 based toolchain as well.
No surprise really given none of the m68k macros use the persistent arg.

If you want to move that signed-off-by and put the warning text in
the message description I will carry this in the m68knommu git tree.

Thanks
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

Re: [RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Ezequiel García]

Hi Greg,

2012/4/23 Greg Ungerer <gerg@snapgear.com>:
>
> Normally this signed-off-by goes after your patch description
> (Documentation/SubmittingPatches section 12).

I know. That text wasn't intended to be part of the commit message, it was
to trigger the discussion. I'll resend the patch with a proper commit message,
including the warning.

Same warning will appear on every other arch, since the only one using
flat_set_persistent() is arch/blackfin...
do you think it would be nice to submit that also (to the proper
lists, of course)?

Thanks for the review,
Ezequiel.

Re: [RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Greg Ungerer]

Hi Ezequiel,

On 24/04/12 12:35, Ezequiel García wrote:
> 2012/4/23 Greg Ungerer<gerg@snapgear.com>:
>>
>> Normally this signed-off-by goes after your patch description
>> (Documentation/SubmittingPatches section 12).
>
> I know. That text wasn't intended to be part of the commit message, it was
> to trigger the discussion. I'll resend the patch with a proper commit message,
> including the warning.

Ok, sorry, I thought that was supposed to be the commit message.


> Same warning will appear on every other arch, since the only one using
> flat_set_persistent() is arch/blackfin...
> do you think it would be nice to submit that also (to the proper
> lists, of course)?

Yes, I think that is a good idea.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

[RFC/PATCH] m68k: fix compiler warning by properly inlining flat_set_persistent()

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
To define or to inline that is the question:
The current definition of flat_set_persistent produces a compiler 
warning; arch/sh/ does it in a different way defining it to 
a macro that uses persistent var. IMHO, an inline is easier to read.
---
 arch/m68k/include/asm/flat.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/m68k/include/asm/flat.h b/arch/m68k/include/asm/flat.h
index a0e2907..f9454b8 100644
--- a/arch/m68k/include/asm/flat.h
+++ b/arch/m68k/include/asm/flat.h
@@ -11,6 +11,11 @@
 #define	flat_get_addr_from_rp(rp, relval, flags, p)	get_unaligned(rp)
 #define	flat_put_addr_at_rp(rp, val, relval)	put_unaligned(val,rp)
 #define	flat_get_relocate_addr(rel)		(rel)
-#define	flat_set_persistent(relval, p)		0
+
+static inline int flat_set_persistent(unsigned long relval,
+				      unsigned long *persistent)
+{
+	return 0;
+}
 
 #endif /* __M68KNOMMU_FLAT_H__ */
-- 
1.7.3.4