On Wed, Apr 02, 2008 at 09:32:55AM -0500, James Bottomley wrote:
> On Wed, 2008-04-02 at 08:30 -0600, Matthew Wilcox wrote:
> > On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > > On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > > > BUG_ON() should not do anything in the macro except test for a value, no
> > > > function calling. I think checkpatch.pl checks for this...
> > >
> > > Well, we can agree to differ on this. The camp that wants no side
> > > effects for BUG_ON() does so in case they want to define it to be a nop.
> >
> > That's one argument, but to me, the most important thing is that reading
> > the content of BUG_ON is unnecessary for understanding the function.
> >
> > > OK ... your subsystem tree your call, I suppose. How about the
> > > attached.
> >
> > > -static inline int transport_container_unregister(struct transport_container *tc)
> > > +static inline void transport_container_unregister(struct transport_container *tc)
> > > {
> > > - return attribute_container_unregister(&tc->ac);
> > > + int err = attribute_container_unregister(&tc->ac);
> > > + BUG_ON(err);
> > > }
> >
> > What's wrong with:
> >
> > if (attribute_container_unregister(&tc->ac))
> > BUG();
>
> You've lost the unlikely designation which is one of the main reasons
> for using BUG_ON(). Most people who write in this form also forget it
> leading to a heap of suboptimal jump prediction code in the kernel and
> another reason not to encourage it.
Most people who think that they need "unlikely" are also usually wrong
:)
This is on a register/unregister path, nothing "fast" about it at all,
so please don't be worrying about "unlikely" for stuff like this.
I prefer the form mentioned by Matthew above please.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
