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

Remove THIS macro #1756

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove THIS macro #1756

wants to merge 3 commits into from

Conversation

tom-overton
Copy link
Collaborator

@tom-overton tom-overton commented Dec 11, 2024

When OoT hit 100%, a PR to remove the THIS macro followed shortly thereafter: zeldaret/oot#1047
This is just the MM equivalent of that change. Like Fig said in the original PR, this was useful to have around when we were decompiling things, but it's not really necessary now, and it hides exactly what is going on with that upcast.

Some stray thoughts I had:

  • Should we change the documentation? I did as part of this PR, but I can undo that change if it's desired. I really wasn't sure if we should leave it as-is for historical purposes or modify it to better suit the current repo.
  • There were a few instances where THIS was being used inline (which were probably fake matches), particularly in the z_fbdemo_X files. I replaced them too, but I wasn't sure if that was the best approach.

I wrote a really crappy script to do this automatically for me, so hopefully nothing slipped through or broke.

@hensldm
Copy link
Collaborator

hensldm commented Dec 11, 2024

Should we change the documentation? I did as part of this PR, but I can undo that change if it's desired. I really wasn't sure if we should leave it as-is for historical purposes or modify it to better suit the current repo.

Tbh, my thoughts would be leave it as is for historical purposes. My guess is that we will eventually do what OoT did and just remove it (though keeping the object tutorial is probably still worth it).

Copy link
Contributor

@fig02 fig02 left a comment

Choose a reason for hiding this comment

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

no opinions on tutorial

@tom-overton
Copy link
Collaborator Author

Ended up reverting the changes to the documentation; let's just leave it as-is for now unless someone feels super strongly about it.

@AngheloAlf AngheloAlf added Merge-ready All reviewers satisfied, just waiting for CI and removed Needs-second-approval Second approval Needs-first-approval First approval labels Dec 11, 2024
@hensldm
Copy link
Collaborator

hensldm commented Dec 12, 2024

Could you add it back to 'style.md'? The style guide should continue to be updated as needed imo.

@hensldm hensldm added the Waiting-for-author Author needs fix to conflicts or address reviews label Dec 12, 2024
@tom-overton
Copy link
Collaborator Author

Oops, I just reverted everything in docs and missed that style.md was supposed to remain the way it was. I re-did those changes.

@tom-overton tom-overton removed the Waiting-for-author Author needs fix to conflicts or address reviews label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean up Merge-ready All reviewers satisfied, just waiting for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants