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

Define field script opcode constants #2005

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rayrobdod
Copy link

Description

Also replace the magic number opcodes in field script command macros with these new constants

Discord contact info

yoshord

Replace the magic number opcodes in field script command macros with
these new constants
Copy link
Collaborator

@mrgriffin mrgriffin left a comment

Choose a reason for hiding this comment

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

I really like the idea here and I think it will reduce mistakes, I just have a few comments on style :)

Comment on lines +4 to +8
.ifndef ALLOCATE_SCRIPT_CMD_TABLE
enum \constant
.else
.4byte \value
.endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think .if/.ifne would be clearer than .ifdef/.ifndef (and are necessary with an explicit .set ALLOCATION_SCRIPT_CMD_TABLE, 0), e.g.:

Suggested change
.ifndef ALLOCATE_SCRIPT_CMD_TABLE
enum \constant
.else
.4byte \value
.endif
.if ALLOCATE_SCRIPT_CMD_TABLE
.4byte \value
.else
enum \constant
.endif

@@ -1,52 +1,54 @@
.include "data/script_cmd_table.inc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to be explicit here:

Suggested change
.include "data/script_cmd_table.inc"
.set ALLOCATE_SCRIPT_CMD_TABLE, 0
.include "data/script_cmd_table.inc"



enum_start
.ifdef ALLOCATE_SCRIPT_CMD_TABLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.ifdef ALLOCATE_SCRIPT_CMD_TABLE
.if ALLOCATE_SCRIPT_CMD_TABLE


.ifdef ALLOCATE_SCRIPT_CMD_TABLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.ifdef ALLOCATE_SCRIPT_CMD_TABLE
.if ALLOCATE_SCRIPT_CMD_TABLE

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.

2 participants