Re: [PATCH 1/2] e1000: fix netpoll with NAPI

["John W. Linville" <linville@tuxdriver.com>]

On Thu, Jun 08, 2006 at 10:23:56AM -0700, Mitch Williams wrote:
> On Wed, 2006-06-07 at 11:54 -0700, John W. Linville wrote:
> 
> > Pedantic objection, but I think this would read easier w/o the extra
> > newline before disable_irq.
> 
> Heh.  I prefer to have a newline between declarations and code.  The

Normally I would agree.  But in this case, I find the distraction of
the random newline after the #else to be more compelling.

> real problem is the position of the #ifdef -- that's what makes it
> difficult to read.  The other solution would be
> {
>         struct e1000_adapter *adapter = netdev_priv(netdev);
> #ifdef CONFIG_E1000_NAPI
> 	int budget = 0;
> #endif
> 
> 	disable_irq(adapter->pdev->irq);
> 
> #ifdef CONFIG_E1000_NAPI
> 	< all that stuff >
> #else
> 	<rest of the stuff >
> #endif
> }
> 
> Which I think is worse to read.

I presume it is the double #ifdef that you find objectionable?
I don't really like it, but at least that idiom is quite common.

Given that the disable_irq appears in both code paths (almost by
necessity), there is a certain appeal to having it outside of the
#ifdef block.  That seems more maintainable.

To me, the idiomatic #ifdef placement seems more readable, if for no
other reason than familiarity.  I suppose we can agree to disagree.

John
-- 
John W. Linville
linville@tuxdriver.com