Re: CVS commit: src/sys/miscfs/genfs
On Wed, Mar 12, 2014 at 09:39:23AM +, Juergen Hannken-Illjes wrote: Module Name: src Committed By: hannken Date: Wed Mar 12 09:39:23 UTC 2014 Modified Files: src/sys/miscfs/genfs: layer_vnops.c Log Message: 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... - Jukka.
Re: CVS commit: src/sys/miscfs/genfs
Taylor R Campbell campbell+netbsd-source-change...@mumble.net writes: Date: Wed, 12 Mar 2014 16:16:32 +0200 From: Jukka Ruohonen jruoho...@iki.fi On Wed, Mar 12, 2014 at 09:39:23AM +, 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))'. FWIW, I agree with Taylor. pgpSzcZOop3yS.pgp Description: PGP signature
Re: CVS commit: src/sys/miscfs/genfs
On Wed, 12 Mar 2014, Greg Troxel wrote: Taylor R Campbell campbell+netbsd-source-change...@mumble.net writes: Date: Wed, 12 Mar 2014 16:16:32 +0200 From: Jukka Ruohonen jruoho...@iki.fi On Wed, Mar 12, 2014 at 09:39:23AM +, 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))'. FWIW, I agree with Taylor. Me, too. But I'd rather that we had the equivalent ISCLR() macro, too, to remove another negation/complement. BTW, why do we have man pages for isset(9)/isclr(9)/setbit(9)/clrbit(9) and then a separate page for SET(9)? And why do we not have manlinks for ISSET(9) and ISCLR(9)? And no cross-refs? :) :) :) - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/miscfs/genfs
Paul Goyette p...@whooppee.com writes: Me, too. But I'd rather that we had the equivalent ISCLR() macro, too, to remove another negation/complement. So is ISCLR when passed two bits true if both are clear, or if it's just not the case that both are set? Arguably this points out that the ISSET docs should explain about using 2 bits at once. pgpZNQF7rHjXV.pgp Description: PGP signature
Re: CVS commit: src/sys/miscfs/genfs
Taylor R Campbell campbell+netbsd-source-change...@mumble.net wrote: Date: Wed, 12 Mar 2014 16:16:32 +0200 From: Jukka Ruohonen jruoho...@iki.fi On Wed, Mar 12, 2014 at 09:39:23AM +, 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) ... 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. -- Mindaugas
Re: CVS commit: src/sys/miscfs/genfs
On Thu, Mar 13, 2014 at 12:32:38AM +0900, Mindaugas Rasiukevicius wrote: Taylor R Campbell campbell+netbsd-source-change...@mumble.net wrote: Date: Wed, 12 Mar 2014 16:16:32 +0200 From: Jukka Ruohonen jruoho...@iki.fi On Wed, Mar 12, 2014 at 09:39:23AM +, 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: da...@l8s.co.uk
Re: CVS commit: src/sys/miscfs/genfs
On Wed, Mar 12, 2014 at 08:11:26AM -0700, Paul Goyette wrote: Me, too. But I'd rather that we had the equivalent ISCLR() macro, too, to remove another negation/complement. So is ISCLR when passed two bits true if both are clear, or if it's just not the case that both are set? Arguably this points out that the ISSET docs should explain about using 2 bits at once. Or might we need ISANYCLR() vs ISALLCLR() (technically, AREALLCLR() but that's just an English grammar nit!) :) By the time you finish with that you just have an alternate and less legible expression syntax... If it were me I'd rewrite the offending code to not use flag bits at all - bitfields generate equally good code in modern compilers and are much less likely to cause confusion. -- David A. Holland dholl...@netbsd.org
