Date: Thu, 3 Aug 2017 13:21:56 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Hans Petter Selasky <hps@selasky.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r321920 - head/sys/sys Message-ID: <20170803122015.Q1093@besplex.bde.org> In-Reply-To: <20170802135455.GG1700@kib.kiev.ua> References: <201708021014.v72AEHEk061037@repo.freebsd.org> <f7a4a90c-f1d8-b381-27fe-3cf76b574a29@selasky.org> <37abc595-c80e-a8da-04a8-815f42c634de@selasky.org> <20170802135455.GG1700@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 2 Aug 2017, Konstantin Belousov wrote: > On Wed, Aug 02, 2017 at 02:38:50PM +0200, Hans Petter Selasky wrote: >> On 08/02/17 14:36, Hans Petter Selasky wrote: >>> On 08/02/17 12:14, Konstantin Belousov wrote: >>>> +#define major(x) ((int)((dev_t)(x) >> 32)) /* major number */ >>>> +#define minor(x) ((int)((x) & 0xffffffff)) /* minor number */ >>>> +#define makedev(x, y) (((dev_t)(x) << 32) | (y)) /* create >>>> dev_t */ >>> >>> One more comment on this issue: >>> >>> I think makedev(x, y) should be declared like this, to avoid issues when >>> "y" is negative: >>> >>> #define makedev(x, y) (((dev_t)(x) << 32) | (unsigned int)(y)) >>> /* create dev_t */ I think this has a lot of style bugs: - long line constructed by blind expansion - verbose spelling of 'unsigned' in the expansion to make the long line longer - banal comment to make the long line. The comment on this line is not quite as banal as the ones on the preceding 2 line -- those do less than echo the code. >> And you'll probably want a final wrapping dev_t cast aswell. 128-bit >> numbers are not yet there. >> >> #define makedev(x, y) ((dev_t)(((dev_t)(x) << 32) | (unsigned >> int)(y))) >> > /* create dev_t */ You mean 128-bit ints. > I agree with the usefulness of the y cast to unsigned type, but I am not sure > what is the use of final dev_t cast. By the usual arithmetic conversion > rules, the final type of the '|' is the highest rank type of the operands. > Something unusual can only happen if int is wider than dev_t. So it happens with 128 ints (or 65-bit ints) if dev_t remains 64 bits. > So I am going to commit the following update. Almost OK. The necessary casts and parentheses are already ugly enough. This is the implementation, so it can assume that int is 32 bits and dev_t is 64 bits and not have the complexity to support the general case. Ints are probably assumed to be 32 bits in a few thousand similar definitions and a few million lines of code just in FreeBSD sources. > diff --git a/sys/sys/types.h b/sys/sys/types.h > index fce57e412ed..30a08724443 100644 > --- a/sys/sys/types.h > +++ b/sys/sys/types.h > @@ -366,7 +366,7 @@ __bitcount64(__uint64_t _x) > > #define major(x) ((int)((dev_t)(x) >> 32)) /* major number */ > #define minor(x) ((int)((x) & 0xffffffff)) /* minor number */ > -#define makedev(x, y) (((dev_t)(x) << 32) | (y)) /* create dev_t */ > +#define makedev(x, y) (((dev_t)(x) << 32) | (unsigned)(y)) /* create dev_t */ > > /* > * These declarations belong elsewhere, but are repeated here and in You fixed the long line by not spelling 'unsigned' verbosely and not aligning the comments uniformly. It would be better to remove the comments. makedev() actually has a man page (a FreeBSD addition makedev(3)). This could have been better, and is now out of date. The largest error is that major() is still documented to return a value between 0 and 255. This reduction prevented makedev() being the inverse of major()+minor() on arbitrary args. The man page doesn't claim that it is, and barely gives a hint that makedev() can be used to synthesize a dev_t from (x, y) provided 0 <= x <= 255. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170803122015.Q1093>