Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RTM] Fix triple newlines left over from removing address comments, and other improvements #534

Merged
merged 16 commits into from Jun 30, 2018

Conversation

Rangi42
Copy link
Member

@Rangi42 Rangi42 commented Jun 25, 2018

Example:

Function1337: ; 1337
    ret
; 1338

Function1338: ; 1338
    ...

became:

Function1337:
    ret


Function1338:
    ...

so this reformats it to:

Function1337:
    ret

Function1338:
    ...

@mid-kid
Copy link
Member

mid-kid commented Jun 25, 2018

I actually removed the entire line if I removed the address comment on an otherwise blank line.
It's probable, however that things like

Function1337: ; 1337
    ret

; 1338

Function1338: ; 1338
    ...

became

Function1337: ; 1337
    ret


Function1338: ; 1338
    ...

I thought it was fine because there's a lot of other places where two newlines are used between functions for some reason.
Any two newlines separating things in constants/ already existed before I went through them. I didn't touch those at all.

Copy link
Member

@mid-kid mid-kid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually happy to see double blank lines between functions used less, as they're rather arbitrary.

@@ -232,7 +230,6 @@ AI_Types:
jr .checkmove

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still three newlines here?

@@ -260,7 +257,6 @@ AI_Offensive:
jr .checkmove

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well? Not going to point all of them out, but there's more.

@Rangi42
Copy link
Member Author

Rangi42 commented Jun 25, 2018

I don't know why those were missed the first time, but now they're done.

(events_2.asm was only separate because scripting.asm intervenes; but other files have a similar "code / INCLUDE self-contained code / more code" structure to this)
@Rangi42 Rangi42 changed the title Fix triple newlines left over from removing address comments [RTM] Fix triple newlines left over from removing address comments, and other improvements Jun 25, 2018
@@ -70,5 +70,5 @@ CorrectNickErrors::
db "<ROUTE>", "<GREEN>" + 1
db "<ENEMY>", "<ENEMY>" + 1
db "<MOM>", "<TM>" + 1
db "<ROCKET>", "┘" + 1
db "<ROCKET>", " "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" " has no relation other than being one after. If border tiles don't count as control characters the problem is the labelling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess border tiles can count as control characters, then.

CARDFLIP_LIGHT_OFF EQU $ef
CARDFLIP_LIGHT_ON EQU $f5
CARDFLIP_LIGHT_OFF EQUS "\"♂\"" ; $ef
CARDFLIP_LIGHT_ON EQUS "\"♀\"" ; $f5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an assembler bug. EQU "♂" should be allowed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True; but until then the escapes are necessary, and were already used in some other files.

home/audio.asm Outdated
@@ -50,8 +50,6 @@ UpdateSound::

_LoadMusicByte::
; wCurMusicByte = [a:de]
GLOBAL LoadMusicByte
Copy link
Contributor

@yenatch yenatch Jun 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was here because BANK() used to not flag labels for import, but it was fixed a while ago.

@yenatch yenatch merged commit 2641f93 into pret:master Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants