Re: CVS commit: src/sys/miscfs/genfs

[David Laight <david%l8s.co.uk@localhost>]

On Thu, Mar 13, 2014 at 12:32:38AM +0900, Mindaugas Rasiukevicius wrote:
> Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost> 
> wrote:
> >    Date: Wed, 12 Mar 2014 16:16:32 +0200
> >    From: Jukka Ruohonen <jruohonen%iki.fi@localhost>
> > 
> >    On Wed, Mar 12, 2014 at 09:39:23AM +0000, Juergen Hannken-Illjes wrote:
> >    > Restructure layer_lock() to always lock before testing for dead node.
> >    > Use ISSET() to test flags, add assertions.
> > 
> >    As I wrote in the manual page, I'd rather see ISSET(3) et. al.
> > disappear, i.e. these obscure rather than clarify...
> > 
> > I disagree.  Phrases like `(vp->v_iflag & (VI_XLOCK | VI_CLEAN)) == 0'
> > make my head's parser stumble -- there are just enough complements to
> > juggle that it overwhelms my brain registers for the fast path.  I'd
> > rather read `!ISSET(vp->v_iflag, (VI_XLOCK | VI_CLEAN))'.
> 
> I disagree.  For kernel developers, that kind of bitwise arithmetics and
> masking ought to be intuitive.  If there is more logic and it gets long,
> then separate it:
> 
> const bool foobar = (mask & (FOO | BAR)) == 0;
> const bool baz = (mask & BAZ) != 0;
> 
> if (foobar && baz) ...

Except you really don't want the compiler to convert the value to
a 'bool'.

> ISSET() is somewhat okay (although I do not use it), but I particularly
> dislike __BIT() as I forget whether the 1st bit is n = 0 or whether this
> API tries to be fancy and it is n = 1.  1U << n is just straigtforward.

Or number from the other end...

Indeed, I have to go away and find the definitions and then realise
that they are just longhand!

I don't normally compare bit masking against zero, just:
        if (var & BIT)
or
        if (!(var & BIT))
to me they read better that way.

        David

-- 
David Laight: david%l8s.co.uk@localhost