Date: Thu, 01 Oct 2009 14:43:45 -0400 From: Coleman Kane <cokane@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, Dag-Erling =?ISO-8859-1?Q?Sm=F8rgrav?= <des@des.no>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r197654 - head/sys/dev/if_ndis Message-ID: <1254422625.4255.46.camel@localhost> In-Reply-To: <20091002040903.U21917@delplex.bde.org> References: <200910010243.n912hpSM034846@svn.freebsd.org> <86eipno12p.fsf@ds4.des.no> <20091002002534.D21507@delplex.bde.org> <1254418346.4255.31.camel@localhost> <20091002040903.U21917@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Fri, 2009-10-02 at 04:20 +1000, Bruce Evans wrote:
> On Thu, 1 Oct 2009, Coleman Kane wrote:
>
> > On Fri, 2009-10-02 at 00:36 +1000, Bruce Evans wrote:
> >> On Thu, 1 Oct 2009, [utf-8] Dag-Erling Smørgrav wrote:
> >>
> >>> Coleman Kane <cokane@FreeBSD.org> writes:
> >>>> - if (sc->ndis_80211 && vap)
> >>>> + if ((sc->ndis_80211 != NULL) && (vap != NULL))
> >>>
> >>> sc->ndis_80211 is an int. NULL is a pointer.
> >>
> >> Also, the number of style bugs was doubled on (almost?) every changed line
> >> by adding 2 sets of unnecessary parentheses.
> >>
> >> Bruce
> >
> > Re-read style(9) more closely.
>
> Do I need to read it at all :-).
I meant that in the past-tense first-person manner, sorry, not trying to
tell anyone what to do. ;) Should have written "I re-read style(9)...".
>
> > Yes... the extra parentheses are superfluous, and should therefore be
> > removed. However, the current rev, which looks like this:
> >
> > if ((sc->ndis_80211 != 0) && (vap != NULL))
> >
> > doesn't help the author shoot themselves in the foot as violating the
> > "explicitly compare values to zero" rule did in the earlier revision.
>
> Actually I needed to count the style bugs more carefully -- 2 implicit
> comparisons with 0 or NULL (unless the first one is really boolean),
> but I only counted 1, while I counted 2 for the extra parentheses.
I think you're right about the ndis_80211. I got thrown off by the first
usage of it in the file which reads:
sc->ndis_80211++;
But it looks like 1) It is tested elsewhere as a boolean, and 2) That
statement really means sc->ndis_80211 = 1 (or = TRUE).
>
> > I'll heed the request of the second-to-last paragraph of style(9) on
> > this particular change, not churning the SVN repo further, and make a
> > mental note for later.
>
> Thanks. I forgot about that paragraph being there. I think churning
> repos doesn't matter much now if it ever did, but churning sources makes
> their history hard to understand.
>
> Bruce
--
Coleman Kane
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)
iEYEABECAAYFAkrE+GEACgkQcMSxQcXat5ccBACeNFaorAGbJhBRJdEDb8gOKH24
7G4AmwXTnh1TJPOF7OjBXlWxnIH0BVXj
=piNZ
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1254422625.4255.46.camel>