Fix MultiplyNumbers macro

[ISSOtm]

It used to shift the number left 11 times, not multiply it.

[ahrnbom]

Wow, this was a big oversight.

In the push bc line, modify the comment to mention that we can use both B and C, not just B.

In the macro, the line ld bc, (\2) << 8 | (\1) is not readable for beginners. I suggest either adding a comment explaining what happens (like ; equivalent to ld b, \2 followed by ld c, \1 or just do

ld b, \2
ld c, \1 ; a more efficient implementation would be "ld bc, (\2) << 8 | (\1)"

or something along those lines.

[ahrnbom]

As of 78910fb, I ended up fixing it myself. It's a slightly different solution than yours (when it comes to the macro) which I think is easier for beginners to understand. Thanks very much for pointing out this (and the other!) flaws.

[JL2210]

@ahrnbom You didn't actually fix it, you just documented it as fixed. It's still present in the latest Git revision.

[ahrnbom]

No, the error has been fixed, just not in the way suggested by the pull request.

The current code in the book is

MultiplyNumbers: MACRO
    push bc

    ld a, \1 ; Store X in A 
    ld b, (\2-1) ; Store Y-1 in B
    ld c, a 
    
.loop\@:
    add c ; Add X
    dec b
    jr nz, .loop\@
    
    pop bc     
ENDM

which I have confirmed to work.

[JL2210]

There must be another instance of MultiplyNumbers because I saw an incorrect version in what was built from Git a few days ago.

…

On Sun, May 31, 2020, 1:29 AM Martin Ahrnbom ***@***.***> wrote: No, the error has been fixed, just not in the way suggested by the pull request. The current code in the book is MultiplyNumbers: MACRO push bc ld a, \1 ; Store X in A ld b, (\2-1) ; Store Y-1 in B ld c, a .loop\@: add c ; Add X dec b jr nz, .loop\@ pop bc ENDM which I have confirmed to work. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJILKCOYCH3ZM43BJE3RURTRUHTL7ANCNFSM4KDCOJPQ> .