Date: Sun, 30 Nov 2014 00:49:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Takahashi Yoshihiro <nyan@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r275241 - head/sys/boot/pc98/boot2 Message-ID: <20141129235225.C1402@besplex.bde.org> In-Reply-To: <201411291222.sATCMWJv060917@svn.freebsd.org> References: <201411291222.sATCMWJv060917@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 29 Nov 2014, Takahashi Yoshihiro wrote:
> Log:
> MFi386: r275059, r275061, r275062 and r275191 (by rdivacky)
>
> Shrink boot2 by a couple more bytes.
These changes don't look too good to me. One is just a bug.
> Modified: head/sys/boot/pc98/boot2/boot2.c
> ==============================================================================
> --- head/sys/boot/pc98/boot2/boot2.c Sat Nov 29 11:50:19 2014 (r275240)
> +++ head/sys/boot/pc98/boot2/boot2.c Sat Nov 29 12:22:31 2014 (r275241)
> ...
> @@ -533,6 +534,7 @@ parse()
> const char *cp;
> unsigned int drv;
> int c, i, j;
> + size_t k;
This still has a style bug (unsorting).
>
> while ((c = *arg++)) {
> if (c == ' ' || c == '\t' || c == '\n')
> @@ -555,7 +557,7 @@ parse()
> #if SERIAL
> } else if (c == 'S') {
> j = 0;
> - while ((unsigned int)(i = *arg++ - '0') <= 9)
> + while ((i = *arg++ - '0') <= 9)
> j = j * 10 + i;
> if (j > 0 && i == -'0') {
> comspeed = j;
This used to work. The cast was an obfuscated way of checking for
characters less than '0'. Removing the cast breaks the code. Garbage
non-digits below '0' (including all characters below '\0') are now
treated as digits.
This still asks for the pessimization of promoting *arg++ to int before
checking it. The compiler might be able to unpessimize this, but large
code shouldn't be asked for. The following should be better:
uint8_t uc;
...
/* Check that the compiler generates 8-bit ops. */
while ((uc = *arg++ - '0') <= 9)
j = j * 10 + uc;
if (j > 0 && uc == (uint8_t)-'0') {
comspeed = j;
I don't see a correct way to avoid the second check of uc.
Subtracting '0' and using the unsigned obfuscation to check for digits
is used in several other places:
drv = *arg - '0';
if (drv > 9)
...;
This works since 'drv' is unsigned. Drive numbers above 9 are not supported,
so uint8_t could be used here, saving 1 byte. At least 1 more byte can be
saved in each of 'drv == -1' and 'if (drv == -1)'. 'drv' would probably
need to be promoted before use; it is only used once to initialize
dsk.drive. It can be promoted there using no extra bytes by statically
initializing dsk.drive to 0 and storing only 8 bits from 'drv'. The total
savings should be 4-5 bytes.
dsk.unit = *arg - '0';
if (arg[1] != ',' || dsk.unit > 9)
...;
dsk.unit is also unsigned. Unit numbers above 9 are not supported, so
uint8_t should work here too, but since dsk.unit probably needs to be
a 32-bit type and there is no local variable like 'drv', the savings are
not as easy.
dsk.slice = WHOLE_DISK_SLICE;
...
dsk.slice = *arg - '0' + 1;
if (dsk.slice > NDOSPART + 1)
...;
Now disk.slice is already uint8_t, so this code should be optimal. However,
when dsk.slice is used it has to be promoted and about 3 bytes are wasted
there. To avoid this wastage, make disk.slice plain int (no need for
unsigned hacks), initialize it all to 0, and store only the low 8 bits
in the above.
There are so many of these that it might be smaller to use a function
that handles the general case. I suspect that this is not in fact smaller,
since it would need complications like passing back the endptr (like
strtol(), not atoi()).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141129235225.C1402>