Re: [Z80asm-devel] Offer to help in development of the Z80 assembler

[Bas Wijnen]

Hello Paul,

Paul White wrote:
> Hi,
> I found and fixed a few problems with the z80 assembler program.  I'd
> like  to offer my help in future development of this program.

Your help is very welcome.  I have a few notes about your changes.  In
general, I would like to use the patch system on savannah, so please
send patches there.  I prefer to have all changes peer-reviewed, so I'll
also post my changes there, so you can comment on them before they are
committed (or anyone else, but I don't think anyone is reading this.  I
wouldn't mind being wrong about that though ;-) ).

Also, please use -p -u in diffs, they get a lot more readable from it.
(You can put the line "diff -up" in ~/.cvsrc to do it automatically when
using cvs diff.)

> In building the project as downloaded, I had a little trouble on my
> FreeBSD 4.9 system, mainly with gnulib/getopts, but I worked it out and
> got things to compile OK.

Good to hear. :-)

> The following patch fixes some other problems I encountered.  One of
> these  can probably be classed as a cosmetic change, allowing a leading
> "A, " in  instructions like "CP A, nn", where currently these
> instructions must be  written as "CP nn", which I think makes
> understanding the written code  more difficult.
> The other fixes are:
> 1. "IM 0", "IM 1" and "IM 2" were producing wrong code.
> 2. "IN A, (C)" was producing wrong code.
> 3. "#include <getopt.h> was changed to '#include "gnulib/getopt.h"' to
> allow compilation.

This should not be the case.  The makefile specifies -Ignulib, which
adds gnulib to the include path, so <file_from_gnulib> should work.  Can
you check if this option is on the command line executed by make?  Or is
the problem that the compiler finds a different version of getopt.h
first (without getopt_long declared in it)?

> >   char * w;   /* PJW */
>
> 1515a1517,1520
>
> >  w = strcasestr(*p, "a,");    /* allow optional leading "A," before
> > object - PJW */
> >   if (w != NULL) *p = w + 2;

I have two problems with this.  First of all, this allows putting "a,"
before _any_ position where a register is read, not just CP and SUB.  So
it would allow for example "LD H,A,L", which would mean the same as "LD
H,L".

Furthermore, it would allow "CP A,4", but not "CP A, 4" or "CP A , 4" or
something.  In other words, it is much more restrictive with respect to
whitespace than the rest of the program.

To fix this nicely, an extra function rd_a_r or so needs to be made,
which allows an extra "A,", and it should be called from CP and SUB
(and, if desired, from the logical operators) instead of rd_r.

> 2106c2111
> <             wrtb (0x46 + 8 * r--);
> ---
>
> >             wrtb (0x46 + 8 * --r);    /* pre-decrement r : PJW */

Agreed.

> 2119c2124
> <                     wrtb (0x40 + 8 * --r);
> ---
>
> >               wrtb (0x78);            /* PJW */

I prefer to write it as wrtb (0x40 + 8 * (A - 1)), to stress that it is
really the same as the line below.  I had forgotten that r was reused
before this line.  Thanks for the catch.

I posted a patch with these and one other fix to the patch tracker.
Please have a look at it.

Thanks,
Bas

--
I encourage people to send encrypted e-mail (see http://www.gnupg.org).
If you have problems reading my e-mail, use a better reader.
Please send the central message of e-mails as plain text
   in the message body, not as HTML and definitely not as MS Word.
Please do not use the MS Word format for attachments either.
For more information, see http://129.125.47.90/e-mail.html

signature.asc
Description: OpenPGP digital signature