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

fileioc: improve garbage collect handling #270

Merged
merged 15 commits into from
Aug 10, 2020
Merged

fileioc: improve garbage collect handling #270

merged 15 commits into from
Aug 10, 2020

Conversation

beckadamtheinventor
Copy link
Contributor

When a gc will be triggered as a result of calling _Arc_Unarc, set OS graphics mode, restore afterwards.
Added ti_SetPostGCHandler((void)(*routine)(void)) so user programs can easily setup graphics again.
Had to replace some of the files in src/include/ to get fileioc to build. Ignore those if you can lol
GC handler functionality has been tested using the attached program.
archive_overloader.zip

@adriweb adriweb changed the title Add gc handler and etc. fileioc: improve garbage collect handling Jul 13, 2020
@adriweb adriweb requested review from jacobly0 and runer112 July 13, 2020 20:21
@adriweb adriweb linked an issue Jul 13, 2020 that may be closed by this pull request
@adriweb adriweb removed a link to an issue Jul 13, 2020
@adriweb
Copy link
Member

adriweb commented Jul 13, 2020

Kinda related to #222

Copy link
Member

@runer112 runer112 left a comment

Choose a reason for hiding this comment

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

  1. The GraphX-oriented default post-GC handler is problematic. The default handler should simply do nothing. The documentation should then note that, if GraphX is being used, gfx_Begin should be registered.
  2. There should be a pre-GC handler, as well. Analagous behavior and documentation (except mention gfx_End instead of gfx_Begin, of course).

src/fileioc/fileioc.asm Outdated Show resolved Hide resolved
src/fileioc/fileioc.asm Outdated Show resolved Hide resolved
src/fileioc/fileioc.asm Outdated Show resolved Hide resolved
src/fileioc/fileioc.h Outdated Show resolved Hide resolved
@beckadamtheinventor
Copy link
Contributor Author

Changed return type required for pre-gc handler. If it returns a non-zero value, the archival which causes the garbage collect will be cancelled.

src/fileioc/fileioc.asm Outdated Show resolved Hide resolved
src/fileioc/fileioc.h Outdated Show resolved Hide resolved
src/fileioc/fileioc.h Outdated Show resolved Hide resolved
@runer112
Copy link
Member

By the way, although I'm bombarding you with change requests, I'd like to state that I appreciate your work on this. After my last few comments are resolved, it looks like everything should be in good shape from my perspective.

@beckadamtheinventor
Copy link
Contributor Author

All comments should be resolved. I think there's another comment, but I don't see it lol.

@mateoconlechuga
Copy link
Member

Looks nice enough. Can you just fix the spacing please?

@mateoconlechuga mateoconlechuga self-requested a review July 19, 2020 04:05
Copy link
Member

@mateoconlechuga mateoconlechuga left a comment

Choose a reason for hiding this comment

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

spacing should use tabs between opcode/operand

@beckadamtheinventor
Copy link
Contributor Author

Done. :P

src/fileioc/fileioc.asm Outdated Show resolved Hide resolved
@jacobly0 jacobly0 force-pushed the llvm branch 2 times, most recently from 30474c9 to c5e4557 Compare August 9, 2020 03:24
@jacobly0 jacobly0 force-pushed the llvm branch 7 times, most recently from 4065b2a to 98d091f Compare August 9, 2020 04:25
; sp + 6 : pointer to routine to be run after. Set to 0 to use default handler.
; return:
; None
pop de
Copy link
Member

Choose a reason for hiding this comment

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

can someone else just double check that this pop order is correct? it seems that it sets up the pointers backwards but maybe I'm just tired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that apparently changed during my last pull lol. Fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, no. Just realised that was an intentional optimization

Copy link
Member

@mateoconlechuga mateoconlechuga Aug 9, 2020

Choose a reason for hiding this comment

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

I'm aware. I don't think it was ever correct; which is why I am asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Dude. I am asking you to look at if you are popping the arguments in the correct order. I know how to write code.

@jacobly0 jacobly0 force-pushed the llvm branch 10 times, most recently from 7788074 to 1933652 Compare August 10, 2020 04:18
@adriweb
Copy link
Member

adriweb commented Aug 10, 2020

well the CI is still in progress, but the toolchain builds...
Anyway, eventually it would be nice to add an autotested example too :)

@adriweb adriweb merged commit aede989 into CE-Programming:llvm Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants