svn commit: r236917 - head/sys/kern

[Pawel Jakub Dawidek]

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 >= 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)) != 0);
> > >>> }
> > >>
> > >> This is backwards.  Apart from using the worst possible (most verbose)
> > >> spelling of `unsigned', it uses a type hack manually optimize away the
> > >> 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 being
> > > 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.
> > 
> > I'm only free to ask you to back out it out.
> 
> I agree that this change should backed out for better. It gains nothing 
> for modern compilers, but makes code reading harder. 

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.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20120613/080770d0/attachment.pgp