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

Use stdbool.h instead of defining custom bool types. #2259

Closed
wants to merge 1 commit into from

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Sep 25, 2024

What does this PR do?

Just modernizes the codebase a little bit as I don't think Premake needs to define its own bools anymore?

@tritao tritao marked this pull request as ready for review September 25, 2024 14:40
Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

This is a tricky PR. I agree with the idea of using bool, true and false where applicable. However, I'm not sure it's applicable to any instances in this PR.

Firstly, the Lua functions return an int not a bool, so we should continue to use an int value - from C23 onwards, true and false will be of type bool not int and also won't be defined in the stdbool.h header. Similarly, the Lua functions that are used, such as lua_dump, take an int not a bool, same logic.

The key issue here is getting type warnings when C23 or later becomes the default. Also, there's more than 6 C files in src/host, so we're obviously not returning TRUE and FALSE on all of the Lua functions, consider just changing them to 1 and 0. If they're internal functions then change the return type to bool - getKernelVersion and getversion in os_getversion.c are internal functions that could return bool instead.

Secondly, Windows API functions use BOOL and we should use TRUE and FALSE as is intended as some functions can have a third value. The same logic should apply to the Objective-C Boolean type used in os_getversion.c.

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

I agree with your requested changes, didn't really think this one through, just put something together that I thought would be a "quick clean up", but as usual reality ends up being a bit more difficult to deal with.

I'm going to drop this PR as I don't have the bandwith at the moment to do it properly, but will have your feedback in mind to clean this up in the future.

@tritao tritao closed this Sep 26, 2024
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.

3 participants