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

[Paul J. White]

Hi Bas,
See my comments below:

At 05:20 PM 3/11/2005 +0100, Bas Wijnen wrote:
> 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 ;-) ).

Thanks for your prompt reply.  I will use savannah in the future.


> 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.)

OK, no problem.


> > 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)?

I'm sure you're correct.  I was just sort of stumbling around, trying 
things to make it work.  I'll look into the Makefile and my system settings 
in more detail and try to figure out what's going on.


> > >   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".

It appears to me that the rd_r routine that I modified is used for ADC, 
ADD, AND, CP, OR, SBC, SUB and XOR only, which all would work OK with a 
leading "A,".  No LD or any other instructions should be affected, as far 
as I can tell.


> 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.

"CP A,4" and "CP A, 4" should work.  You're right about "CP A , 4", though.
Personally, I don't like to see any space before a comma, but I realize 
that others have different coding styles, and agree that this should be 
allowed.


> 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.

I will.  Thanks for your time.
-- Paul White