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

Document gfxalloc.c #2153

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

Document gfxalloc.c #2153

wants to merge 6 commits into from

Conversation

mzxrules
Copy link
Contributor

@mzxrules mzxrules commented Sep 6, 2024

Changes made in this pr are licensed under CC0.

This covers the documentation pass on gfxalloc.c I made in #1972. It'd be ideal if Gfx* tempGfxDisp; Gfx* lockedGfxDisp; could be combined into one struct, but there's one function where the regalloc simply doesn't work.

src/code/gfxalloc.c Outdated Show resolved Hide resolved
src/code/gfxalloc.c Outdated Show resolved Hide resolved
Gfx* Gfx_Open(Gfx* gfx);
Gfx* Gfx_Close(Gfx* gfx, Gfx* dst);
Gfx* Gfx_Open(Gfx* gfxDisp);
Gfx* Gfx_Close(Gfx* gfxDisp, Gfx* gfx);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why "disp" ? afaik that stands for "display"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opted for disp to make the distinction that the first argument should be a display buffer pointer (i.e. POLY_OPA_DISP) rather than any old Gfx*.

Comment on lines 1160 to 1163
Gfx* tempGfxDisp;
Gfx* lockedGfxDisp;

gfxP = Gfx_Open(sp1CC);
gSPDisplayList(OVERLAY_DISP++, gfxP);
tempGfxDisp = Gfx_Open(lockedGfxDisp = POLY_OPA_DISP);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced by the temp/locked naming scheme (nor "disp")

Copy link
Contributor

Choose a reason for hiding this comment

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

The Gfx_Open(lockedGfxDisp = POLY_OPA_DISP); is awful too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gfx_Open(lockedGfxDisp = POLY_OPA_DISP); is unusual yes, but I'd argue that it makes sense to write unusual code to force POLY_OPA_DISP to appear inside the call to Gfx_Open in this case. If you think of the high level picture, that call to Gfx_Open is creating a side effect on POLY_OPA_DISP, it's taking all of the memory it has and giving it to something else.
When you have the assignment above Gfx_Open I feel that you lose a little bit of that connection.

@mzxrules
Copy link
Contributor Author

mzxrules commented Oct 1, 2024

I opted to create GFX_ALLOC_OPEN/GFX_ALLOC_CLOSE macros, since I think it makes the open/close process much cleaner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants