Machine code comment syntax recommendations

[berarma]

Anything that is 2 opcodes like your border colour example and is used more than once is probably better as a macro.

Sometimes it's worth commenting stuff that isn't obvious on a first scan through the code e.g.

inc a ; does not affect carry flag!

inc hl ; does not affect flags!

Both of those can be useful because they don't set flags though of course, you can do them just before a conditional jump for condition you tested earlier on.

Similarly add hl,bc does not set Z, C or P/V but you normally don't need a comment for that since it usually results in a bug if you thought it would set them. (You have to clear carry and use adc hl,bc if you want to set flags). (Been burned one too many times by this to forget about it now I hope).

That's precisely the kind of thing you don't want to comment on. You should already know what the instructions does, or else you wouldn't even be able to write the comment. But there are exceptions, like when a instruction is used for its side effects rather than the direct result.

[ParadigmShifter]

I think it's ok if that is shortly before a conditional jump and the thing which set the flag is further up the code. Each to their own (and I see most of the time I do that in my code I could move the opcode which sets the flags to just before the conditional jump anyway). Sometimes it's handy to do an inc hl just before a conditional jump if another part of the routine jumps directly to the inc hl instruction though - then it can be useful I think.

Commenting out working code and replacing it with a faster version is another good use for a comment

Code: Select all

	;neg
	;add #E2
	cpl
	add #E3 ; same as above but faster :)

Using a magic number is not good there of course Although it's just doing A = 30 - A

[Einar Saukas]

Sorry, I disagre...

Anything that is 2 opcodes like your border colour example and is used more than once is probably better as a macro.

I think that's a bad idea. If your souce code is full of macros (replacing every pair of instructions used more than once) it will be nearly impossible to read.

Another problem is that macros hide important details. For instance this macro:

Code: Select all

UPDATE_BORDER_COLOR(COLOR_RED)

You may assume it's implemented like this:

Code: Select all

ld a,COLOR_RED
ld (BORDCR), a

but later find out it was actually implemented like this:

Code: Select all

ld hl,BORDCR
ld (hl),COLOR_RED

Thus explaining the reason your code was crashing because one of your macros changed register HL when you didn't expect it...

It's better to use macros sparingly, only when really needed.

inc a ; does not affect carry flag!

inc hl ; does not affect flags!

This kind of comment also contradicts everything I wrote

[ParadigmShifter]

I'm not suggesting replacing every set of 2 opcodes used more than once with a macro, duh. I do tend to use a lot of macros though.

Very short routines that aren't worth a call overhead and are used a lot should be replaced by macros though I think.

Saying "you can't be sure if it will change HL" is like saying "I don't read the comments for a function which states which registers are trashed".

I always capitalise my macros and the rest of the opcodes are in lower case so I always know when a macro is being used.

Sometimes I provide the register to be used as a macro argument as well e.g.

Code: Select all

	MACRO SCRTOATTRIBS reg
	ld a, reg

	or #87
	rra
	rra
	srl a
	ENDM

(I think I borrowed that code from you and then made it into a macro btw)

(It converts the high byte of a screen address into the high byte of an attribute address)

RE: Contradicting everything you wrote - that was why I used "sometimes" and not "always". As in "sometimes it's handy to remind yourself that doing an inc hl before a jr c, .label uses the previously set carry flag value and does not depend on the value of hl"

But I see I am in a minority of 1 here

[sn3j]

Similarly add hl,bc does not set Z, C or P/V but you normally don't need a comment for that since it usually results in a bug if you thought it would set them. (You have to clear carry and use adc hl,bc if you want to set flags).

add hl, bc does set the Carry:

Condition Bits Affected

• S is not affected.
• Z is not affected.
• H is set if carry from bit 11; otherwise, it is reset.
• P/V is not affected.
• N is reset.
• C is set if carry from bit 15; otherwise, it is reset.

[ParadigmShifter]

Yup my bad. It wouldn't be much use for >16 bits arithmetic otherwise of course.

It does not set the S or Z flag though which is annoying.

[sn3j]

I sometimes comment my code in C-style, like Field[x,y] += c or List[j] = &Array[j].

[ParadigmShifter]

I always put the entire C code for a routine in as a comment if I convert something from C to ASM... last time I did that was for a binary search IIRC.

From an earlier version of SJOE (has been optimised a bit since)

Code: Select all

;void *
;bsearch (const void *key, const void *base0,
;         size_t nmemb, size_t size,
;         int (*compar)(const void *, const void *))
;{
;	const char *BC = (const char *) (CurrentWordPtr);
;	int DE, cmp;
;	const void *p;
;
;	for (DE = (WordCount); DE != 0; DE >>= 1) {
;		HL = BC + (DE >> 1) * 3;
;		cmp = (*compar)(key, HL);
;		if (cmp == 0)
;			return (void *)p;
;		if (cmp > 0) {	/* key > p: move right */
;			BC = (const char *)HL + 3;
;			DE--;
;		} /* else move left */
;	}
;	return (NULL);
;}


bsearch_word:
    ld de, (WordCount)
    ld bc, (CurrentWordPtr)
.nextiter
    ld a, d
    or e
    jr z, .finishednotfound
    ld h, d
    ld l, e
    or a
    rr h
    rr l
    push de
    ld d, h
    ld e, l
    add hl, hl
    add hl, de
    pop de
    add hl, bc

    push hl
    ld a, (WordToCheck+1)
    cp (hl)
    jr nz, .nomatch
    inc hl
    ld a, (WordToCheck+2)
    cp (hl)
    jr nz, .nomatch
    inc hl
    ld a, (WordToCheck+3)
    cp (hl)
    jr nz, .nomatch
    ; we got a match, and Z flag is clear, return
    pop hl
    ret
.finishednotfound
    or 1 ; clear Z flag
    ret
.nomatch
    pop hl
    jr c, .moveleft
    ld b, h
    ld c, l
    inc bc
    inc bc
    inc bc
    dec de
.moveleft
    or a
    rr d
    rr e
    jr .nextiter

I inlined the comparison function but didn't comment that, sorry Also size is hardcoded to 3.

[Einar Saukas]

I'm not suggesting replacing every set of 2 opcodes used more than once with a macro

You wrote "Anything that is 2 opcodes like your border colour example and is used more than once is probably better as a macro".

I do tend to use a lot of macros though.

Exactly my point.

Saying "you can't be sure if it will change HL" is like saying "I don't read the comments for a function which states which registers are trashed".

Imagine you are looking at some Assembly code with many opcodes, a few routine calls (and perhaps very few macros). The registers affected by these opcodes will be visible on screen. You only need to check and keep in mind the registers affected in a handful of calls and macros, to write good code.

Now imagine you are looking at some Assembly code with lots of macros mixed between opcodes (besides the same routine calls). Now you will have to check and keep in mind everything affected by so many macros and routine calls. It will be much harder to ensure bug-free code.

[Einar Saukas]

I sometimes comment my code in C-style, like Field[x,y] += c or List[j] = &Array[j].

Agreed!

If it's easier to explain your code using a high-level programming language instead of lots of text, by all means use it!

I have commented a large part of Complica DX using pseudo-code instead of regular text, whenever that made it more clear.

[ParadigmShifter]

You wrote "Anything that is 2 opcodes like your border colour example and is used more than once is probably better as a macro".




Exactly my point.




Imagine you are looking at some Assembly code with many opcodes, a few routine calls (and perhaps very few macros). The registers affected by these opcodes will be visible on screen. You only need to check and keep in mind the registers affected in a handful of calls and macros, to write good code.

Now imagine you are looking at some Assembly code with lots of macros mixed between opcodes (besides the same routine calls). Now you will have to check and keep in mind everything affected by so many macros and routine calls. It will be much harder to ensure bug-free code.

Well you are just quoting without thinking about the meaning.

Of course I don't mean any 2 opcodes used more than once is better as a macro, that would be insane.

I think something like this

Code: Select all

	MACRO SETFOUNDMATCHFLAG
	ex af, af'
	ld a, 1
	ex af, af'
	ENDM

	MACRO CLRFOUNDMATCHFLAG
	xor a
	ex af, af'
	ENDM

which sets/clears a flag in the A' register (used if we find three of the same tiles in a row) is better than using the opcodes since it show what the intention is.

A macro is the same as a function it just uses more memory but saves a call/ret. Obviously you need to see what registers are trashed if you use it. It's just a code size/speed tradeoff.

I make no apologies for using a lot of macros... I think it makes code easier to understand.

[Einar Saukas]

Code: Select all

	MACRO SETFOUNDMATCHFLAG
	ex af, af'
	ld a, 1
	ex af, af'
	ENDM

	MACRO CLRFOUNDMATCHFLAG
	xor a
	ex af, af'
	ENDM

Notice that:

• Macro SETFOUNDMATCHFLAG will only set A'=1, preserving A,F,F'.
• Macro CLRFOUNDMATCHFLAG will set A=A', F=F', A'=0, F'=0x44.

That's not the behaviour I would expect when looking at code using these macros. It's exactly the kind of side-effect hidden within lots of macros that I'm concerned about.

I make no apologies for using a lot of macros... I think it makes code easier to understand.

If using lots of macros work for you, by all means keep using them! I'm not trying to convince you otherwise.

I just don't think that's a good advice for most people.

[ParadigmShifter]

Meh I'm not arsed about the flags maybe I need a comment saying so.

It's faster than

LD A, 1; 7T
LD (someflag), A ; 13T

my macro - (15T)

and it preserves value of A

[Einar Saukas]

and it preserves value of A

Macro SETFOUNDMATCHFLAG preserves value of A, but CLRFOUNDMATCHFLAG doesn't.

[ParadigmShifter]

Well yeah but I only do that prior to a ret so it doesn't matter. I usually assume A will be trashed whenever I do something

[ParadigmShifter]

Anyway we're both good programmers I just like to point out that macros aren't necessarily evil.