svn commit: r275241 - head/sys/boot/pc98/boot2

[Bruce Evans]

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