svn commit: r305091 - head/sys/sys

[Bruce Evans]

On Wed, 31 Aug 2016, Mateusz Guzik wrote:

> On Tue, Aug 30, 2016 at 03:35:39PM -0700, Conrad Meyer wrote:
>> On Tue, Aug 30, 2016 at 2:48 PM, Mateusz Guzik <mjg at freebsd.org> wrote:
>>> Author: mjg
>>> Date: Tue Aug 30 21:48:10 2016
>>> New Revision: 305091
>>> URL: https://svnweb.freebsd.org/changeset/base/305091
>>>
>>> Log:
>>>   fd: simplify fd testing in fget_locked by casting to u_int
>>>
>>> Modified:
>>>   head/sys/sys/filedesc.h
>>>
>>> Modified: head/sys/sys/filedesc.h
>>> ==============================================================================
>>> --- head/sys/sys/filedesc.h     Tue Aug 30 21:43:57 2016        (r305090)
>>> +++ head/sys/sys/filedesc.h     Tue Aug 30 21:48:10 2016        (r305091)
>>> @@ -201,7 +201,7 @@ fget_locked(struct filedesc *fdp, int fd
>>>
>>>         FILEDESC_LOCK_ASSERT(fdp);
>>>
>>> -       if (fd < 0 || fd > fdp->fd_lastfile)
>>> +       if ((u_int)fd > fdp->fd_lastfile)
>>>                 return (NULL);
>>>
>>>         return (fdp->fd_ofiles[fd].fde_file);

This was correct.  Now it is obfuscated by a bogus cast.  Let the compiler
do a strength reduction to a single comparison operator iff that is best.
The cast is like using "register".  In 1980's code I wrote lots of such
unsigned casts since compilers were not so good and didn't do the strength
reduction.

>> I notice that fd_lastfile is an 'int'.  Won't this trigger warnings
>> about the differing signedness of the two sides of the comparison?
>> Should fd_lastfile just be u_int as well?  (If not (there is some
>> valid negative value), this change may be invalid.)

Ugh.  fd_lastfile has the correct for a file descriptor (int).  Making it
unsigned would just require many more bogus casts to break warnings when
comparing it with file descriptors.

Almost all uses of unsigned types are errors.  At best, the unsigned type
doubles the range of non-negative values and gives arithmetic modulo 2**N
instead of normal arithmetic.

> It is -1 just after inception and is supposed to grow immediately after.
> That is, the table with -1 should never be accessible.
>
> But now that you mention it I agree this is bad style. This
> unnecessarily differs from the check in fget_unlocked so I'll just unify
> it.

Don't break the latter too.  I already had to fix it once there.  It had
2 commits just to move the bogus cast back and forth before I removed it.
following churning:
- created in r89969.  This used 1 bogus cast.  It was copied from a bad
   example in kern_descrip.c.  It did a redundant check that (fd < 0).  I
   had committed fixes for some bad examples like this in kern, but not
   this one.
- I complained about this to Albert, but he didn't understand and changed
   it to the above in r89969.  This works, but it is obscure.  It depends
   on both fd and fdp->fd_lastfile having type int.  So both operands are
   promoted to u_int and unsigned magic works.
- some compilers apparently object to the implicit conversion of the second
   operand.  So it was changed to use 2 bogus casts in r100072.  This works,
   but it still depends on both fd and fdp->fd_lastfile having type int.
   Otherwise the cast might truncate 1 or both of them, or if they are
   smaller than int (perhaps int32_t) then it isn't clear if the cast
   works as intended.
- I got tired of giving lessons and committed what I wanted in r126592.
   See the log message for r126592 for less details.

Bruce