Date: Wed, 13 Jun 2012 09:16:13 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Andrey Chernov <ache@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, src-committers@FreeBSD.ORG, svn-src-all@FreeBSD.ORG, svn-src-head@FreeBSD.ORG Subject: Re: svn commit: r236917 - head/sys/kern Message-ID: <20120613071613.GA1386@garage.freebsd.pl> In-Reply-To: <20120613040900.GA88971@vniz.net> References: <201206112017.q5BKHKsW007722@svn.freebsd.org> <20120612114254.V1572@besplex.bde.org> <20120612115117.GB1372@garage.freebsd.pl> <20120613115111.R5323@besplex.bde.org> <20120613040900.GA88971@vniz.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--cWoXeonUoKmBZSoM
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Jun 13, 2012 at 08:09:01AM +0400, Andrey Chernov wrote:
> On Wed, Jun 13, 2012 at 12:37:50PM +1000, Bruce Evans wrote:
> > >> On Mon, 11 Jun 2012, Pawel Jakub Dawidek wrote:
> > >>> - KASSERT(fd >=3D 0 && fd < fdp->fd_nfiles,
> > >>> + KASSERT((unsigned int)fd < fdp->fd_nfiles,
> > >>> ("file descriptor %d out of range (0, %d)", fd, fdp->fd=
_nfiles));
> > >>> return ((fdp->fd_map[NDSLOT(fd)] & NDBIT(fd)) !=3D 0);
> > >>> }
> > >>
> > >> This is backwards. Apart from using the worst possible (most verbos=
e)
> > >> spelling of `unsigned', it uses a type hack manually optimize away t=
he
> > >> test for fd being < 0. The compiler will do this "optimization"
> > >> automatically if it is any good (or undo it if it is not), so all the
> > >> hack does is obfuscate the test. With the verbose spelling of u_int,
> > >> it even takes more space.
> > >
> > > Well, to be honest I presonally would prefer explicit check for fd be=
ing
> > > less than 0, but my impression was that using cast is the most popular
> > > way and I wanted this check to be consistent across our source tree.
> > >
> > > Feel free to change it.
> >=20
> > I'm only free to ask you to back out it out.
>=20
> I agree that this change should backed out for better. It gains nothing=
=20
> for modern compilers, but makes code reading harder.=20
Maybe I'll try again. I much prefer checking fd explicitly for being
less than zero than using cast. The thing is using cast is the most
popular method for that check. I decided to use that method as it was
the least discruptive way of gaining consistency. For me, when I was
reading the code it was more confusing to see different methods of
checking if fd is valid than to see less pretty, but consistent, method
everywhere.
Backing it out would mean that we will use better method in some places,
but reintroduce inconsistency and I don't want to do that. I also don't
want to convert all such checks to more pretty method, as people may
work in this area and I'll just add more conflicts to them to resolve.
I'm experimenting with some changes in perforce. If the experiment
succeeds I'll end up changing a lot of code in this area, which will
allow me to convert all the checks to the preferred method.
If in the meantime one of you would like to change it, be prepare that
people might not be happy and can start to scream. I'll try not to.
--=20
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
--cWoXeonUoKmBZSoM
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAk/YPj0ACgkQForvXbEpPzSxNgCaAgqHdbqH8KABZNZzwZRNeJ7c
5tUAoOJT1Dr8t1c5MScFnTOb63/joZG6
=QX90
-----END PGP SIGNATURE-----
--cWoXeonUoKmBZSoM--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120613071613.GA1386>