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

Experimental SDL Console #5027

Draft
wants to merge 25 commits into
base: develop
Choose a base branch
from
Draft

Experimental SDL Console #5027

wants to merge 25 commits into from

Conversation

dhthwy
Copy link
Contributor

@dhthwy dhthwy commented Nov 15, 2024

It's set to build it by default and disables the other ones for now. Still in draft.

@ab9rf
Copy link
Member

ab9rf commented Nov 15, 2024

You're inconsistent in your usage of class versus struct in a lot of this code. They are not interchangeable (as the build log demonstrates) and it might be best if you picked a consistent approach and stuck with it. My personal preference here is to use struct for types that are mostly "pure data containers" and class for types that have significant behaviors attached to them.

@dhthwy
Copy link
Contributor Author

dhthwy commented Nov 15, 2024

True my intent aligns with your preference. GCC 14 doesn't warn me about that, it even missed a problem with initialization order that MSVC caught.

Anyway, it needs to use DFSDL, but I'll probably just add a symbol resolver function to it for now.

This needs some infrastructure work (some basic layout machinery would be nice). And I'm not 100% sure if the design is sound. If anyone happens to see anything awful, feel free to point it out.

@dhthwy
Copy link
Contributor Author

dhthwy commented Nov 16, 2024

Alright, I think I fixed that inconsistency. Of course, it doesn't build until I make it bind to sdl at run time.

There are other warts (member variables declared at the start of an object definition vs end), some that are public that should be private, etc. You get used to looking at the same code and these types of warts start to look normal.

@dhthwy dhthwy marked this pull request as draft November 16, 2024 03:55
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