Date: Wed, 31 Aug 2016 16:32:26 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Conrad Meyer <cem@freebsd.org>, Mateusz Guzik <mjg@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r305091 - head/sys/sys Message-ID: <20160831155441.V1481@besplex.bde.org> In-Reply-To: <20160830224104.GA16151@dft-labs.eu> References: <201608302148.u7ULmAKm073958@repo.freebsd.org> <CAG6CVpXwtKMp%2BVApeG-cTjVfGtH3DjKyKKPrmPe4fNZdAGNcEQ@mail.gmail.com> <20160830224104.GA16151@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160831155441.V1481>