On Mon, 10 May 2004, Eric Auer wrote:
> You optimized less than 0x10 bytes but you sent a diff of probably
> far more than 100 lines. Most changes involve removing comments, changing
> indendation, replacing code like "foo = bar; if (foo == ...)" by
> code like "if ((foo = bar) == ...)" which is less readable but can
> compile to exactly the same thing depending on your compiler and so on!
>
> So just sent us that 10 line patch which saves those precious 5 bytes
> in ONE mail and send the 100 line patch which just changes layout and
> makes code less readable in ANOTHER mail. Then I can delete the OTHER
> mail and admire the 5 byte savings of the ONE mail.
yes! -- exactly right here. Far too much changed for so little gain.
The fact is that this patch is still much too big because it contains a
mixture of many different things. incl. formatting and optimization. The
only part of this patch that really matters is:
@@ -832,10 +820,7 @@ dispatch:
case 0x38:
{
UWORD cntry = lr.AL;
-
- if (cntry == 0)
- cntry = (UWORD) - 1;
- else if (cntry == 0xff)
+ if (cntry == 0xff)
cntry = lr.BX;
if (0xffff == lr.DX)
@@ -846,11 +831,13 @@ dispatch:
}
else
{
+ if (cntry == 0)
+ cntry--;
/* Get Country Information */
if ((rc = DosGetCountryInformation(cntry, FP_DS_DX)) < 0)
goto error_invalid;
/* below here is already formatting stuff -- shouldn't be in this patch */
/* HACK FIXME */
- if (cntry == (UWORD) - 1)
+ if (cntry == (UWORD)-1)
cntry = 1;
/* END OF HACK */
lr.AX = lr.BX = cntry;
If there was another bugfix in there I missed it because it is snowed in
between other changes.
Furthermore -- I see absolutely no reason for changing
- sft FAR *p = MK_FP(r.ES, r.DI);
to
+ sft FAR *p = FP_PTR (sft, r.ES, r.DI);
this is like changing
sft *p = malloc(sizeof *p);
to
#define TYPE_ALLOC(x) ((x *)malloc(sizeof x))
sft *p = TYPE_ALLOC(sft);
which probably look nice to Arkady but for me malloc(sizeof *p); is much
quicker to understand and more general too (if you change the type of p
you don't need to change the alloc statement). We know and understand what
malloc and MK_FP do. I won't change my opinion that casting pointers (or
anything else really) is bad when it's not necessary. So adding fancy
macros that add casts is the wrong thing to do.
Similary, if you simply split
UWORD callerARG1;
into
UBYTE callerARG0, callerARG1;
you can get cleaner code without fancy lobyte/hibyte macros or ugly casts.
And for
const psp _seg *p = FP_SEG_PTR (const psp, cu_psp);
why not simply use
const psp _seg *p = MK_SP(cu_psp);
where MK_SP works like MK_FP but returns a (void _seg *). That looks a lot
less intrusive to me.
Anyway -- I was playing with _seg pointers myself indeed but then stopped
because it only helps Borland compilers. Sure the kernel needs to
*compile* with Borland, but it only needs to be optimized with Open
Watcom. Introducing _seg pointers is OK (after all you say "this
pointer is not a general far pointer but one with offset 0" which gives
more information) but not so important, as it is purely a matter of style.
Bart
-------------------------------------------------------
This SF.Net email is sponsored by Sleepycat Software
Learn developer strategies Cisco, Motorola, Ericsson & Lucent use to deliver
higher performing products faster, at low TCO.
http://www.sleepycat.com/telcomwpreg.php?From=osdnemail3
_______________________________________________
Freedos-kernel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/freedos-kernel
