Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

[Matt Mackall]

[thanks for the cc]

On Thu, Nov 24, 2005 at 12:47:15PM +0000, Hugh Dickins wrote:
> On Tue, 22 Nov 2005 akpm@osdl.org wrote:
> > 
> > The patch titled
> > 
> >      Shut up warnings in ipc/shm.c
> > 
> > has been added to the -mm tree.  Its filename is
> > 
> >      shut-up-warnings-in-ipc-shmc.patch
> > 
> > 
> > From: Russell King <rmk@arm.linux.org.uk>
> > 
> > Fix two warnings in ipc/shm.c
> > 
> > ipc/shm.c:122: warning: statement with no effect
> > ipc/shm.c:560: warning: statement with no effect
> > 
> > by converting the macros to empty inline functions.  For safety, let's do
> > all three.  This also has the advantage that typechecking gets performed
> > even without CONFIG_SHMEM enabled.
> 
> Sorry to be a nuisance, but I'm a little resistant to this patch.
> Which version(s) of the compiler gives that warning?
> Aren't there 5000 other such stub #defines which should also be changed?
> Or is the problem the rather complex "({0;})" - should that be "0"?
> It seems such clutter to use 6 lines of inline function for each of these.
> Nice try, but I don't buy the typechecking advantage in this case!

Unfortunately Russell didn't tell us which function caused the error
and I can't seem to find a tree that matches his line numbering.
But it looks like it's shm_unlock.

The current ({0;}) seems wrong to me. I'd expect that expression to be
void. Hmm, looks like I'm wrong. It's quite ugly, not to mention confusing.

Andrew introduced it in a patch called "[PATCH] Fix shmem.c
stubs" that did this:

-#define shmem_lock(a, b) /* always in memory, no need to lock */
+#define shmem_lock(a, b, c) ({0;}) /* always in memory, no need to lock */

(shmem_lock changed from void to int a few days before this with
"rlimit-based mlocks for unprivileged users")

I didn't get compile warnings when I introduced tiny-shmem in 2004
(or, for that matter, when I wrote it in 2003) but I do seem to be
getting them now with gcc 4 -W.

So apparently gcc has gotten more picky about such things.

If we're going to start converting such things, I'd almost rather do
something like:

kernel.h:
static inline void empty_void(void) {}
static inline void empty_int(void) { return 0; }
...

mm.h:
#define shm_lock(a, b) empty_int()

The typechecking is nice in theory, but in practice I don't think it
really makes a difference for stubbing things out.

-- 
Mathematics is the supreme nostalgia of our time.

Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

[Muli Ben-Yehuda]

On Thu, Nov 24, 2005 at 08:00:12AM -0800, Matt Mackall wrote:
> If we're going to start converting such things, I'd almost rather do
> something like:
> 
> kernel.h:
> static inline void empty_void(void) {}
> static inline void empty_int(void) { return 0; }
                ^^^^

surely if it's returning an int it should be declared as
static inline int empty_int(void) { return 0; }
?

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

[Matt Mackall]

On Thu, Nov 24, 2005 at 06:07:25PM +0200, Muli Ben-Yehuda wrote:
> On Thu, Nov 24, 2005 at 08:00:12AM -0800, Matt Mackall wrote:
> > If we're going to start converting such things, I'd almost rather do
> > something like:
> > 
> > kernel.h:
> > static inline void empty_void(void) {}
> > static inline void empty_int(void) { return 0; }
>                 ^^^^
> 
> surely if it's returning an int it should be declared as
> static inline int empty_int(void) { return 0; }
> ?

Surely.

-- 
Mathematics is the supreme nostalgia of our time.

Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

[Russell King]

On Thu, Nov 24, 2005 at 08:00:12AM -0800, Matt Mackall wrote:
> Unfortunately Russell didn't tell us which function caused the error
> and I can't seem to find a tree that matches his line numbering.
> But it looks like it's shm_unlock.

To make it completely clear, a lot of my "fix warning" patches are
derived from the ARM Linux kernel autobuilder, which can be found
at http://armlinux.simtec.co.uk/kautobuild/

and I only really look at the latest builds on there.

> The current ({0;}) seems wrong to me. I'd expect that expression to be
> void. Hmm, looks like I'm wrong. It's quite ugly, not to mention confusing.

It seems that ({0;}) is used when something is expected to return zero.
However, if it is used in a void context, gcc 4 generates an annoying
warning.

> mm.h:
> #define shm_lock(a, b) empty_int()
> 
> The typechecking is nice in theory, but in practice I don't think it
> really makes a difference for stubbing things out.

Depends if you end up with "blah is unused" warnings instead.  It's
all round _far_ safer to use the inline function method from that
point of view.

Not that I particularly care, I just wanted to squash some of the
rediculous number of warnings in the kernel and decided to hit the
easy ones.  However, they're turning out to be real pigs instead. 8(

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: + shut-up-warnings-in-ipc-shmc.patch added to -mm tree

[Hugh Dickins]

On Thu, 24 Nov 2005, Russell King wrote:
> On Thu, Nov 24, 2005 at 08:00:12AM -0800, Matt Mackall wrote:
> 
> It seems that ({0;}) is used when something is expected to return zero.
> However, if it is used in a void context, gcc 4 generates an annoying
> warning.

Annoying indeed.

> > mm.h:
> > #define shm_lock(a, b) empty_int()
> > 
> > The typechecking is nice in theory, but in practice I don't think it
> > really makes a difference for stubbing things out.

I'm with Matt on the typechecking here, and at first liked his proposal;
but then dreaded a long line of empty_int_0(), empty_long_minus1(), ...
I suppose we could pass the return value as argument, but I rather lose
interest...

> Depends if you end up with "blah is unused" warnings instead.  It's
> all round _far_ safer to use the inline function method from that
> point of view.
> 
> Not that I particularly care, I just wanted to squash some of the
> rediculous number of warnings in the kernel and decided to hit the
> easy ones.  However, they're turning out to be real pigs instead. 8(

I have nothing constructive suggest, and withdraw my objection to your
patch; though I do hope someone else comes up with a brilliant idea.
Or, does the next version of gcc decide it was all a wrong turning?

Hugh