Re: [announce] new tree: "fix all build warnings, on all configs"

[JÃrn Engel]

On Wed, 22 October 2008 11:47:59 +0200, Ingo Molnar wrote:
> * JÃrn Engel <joern@xxxxxxxxx> wrote:
> > On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
> > >  
> > >  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
> > >  #define fops_get(fops) \
> > > -	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> > > +	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
> > >  #define fops_put(fops) \
> > > -	do { if (fops) module_put((fops)->owner); } while(0)
> > > +	do { if (fops != NULL) module_put((fops)->owner); } while(0)
> > 
> > This, I would argue, makes the code worse.
> 
> In this specific case the issue is that the 'fops' parameter can 
> occasionally be a constant pointer (turning the test into always-true) 
> so the compiler is at least minimally correct at asking the "are you 
> sure you want this" question - which we answer in the affirmative via 
> the explicit NULL check. But these are really nuances.

#define fops_put(fops) do {			\
	if (fops != NULL)			\
		module_put((fops)->owner);	\
} while 0

If the code was written like this, I wouldn't mind your patch at all.
But in the one-liner form, the ' != NULL' makes the difference between
fluent reading and having to actively sort out which piece belongs
where.

And I bet that if it hadn't been a macro but an inline function, you
and checkpatch would have complained about the formatting long ago. ;)

JÃrn

-- 
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/