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 _WIN32 macro everywhere #337

Merged
merged 1 commit into from
Jun 23, 2024

Conversation

@christian-rauch
Copy link
Collaborator

Are you sure that the WIN32 macro does not exist? It may look like a typo because the other macros are named _WIN32, but WIN32 is indeed a defined macro.

This is definitely not a typo, since the macro exists. But I also find it confusing using WIN32 while all other Windows-specific sections of the code use _WIN32.

Can you rephrase your commit message to make clear that this is not a typo but rather to use a common macro or something similar?

@s-trinh s-trinh changed the title Fix WIN32 typo Clarify WIN32 usage Jun 23, 2024
@s-trinh
Copy link
Contributor Author

s-trinh commented Jun 23, 2024

I have updated the commit accordingly. Since it could be specific to MSVC, it is better to keep the original code IMO.

Luckily we have online tool...
https://godbolt.org/z/8EKTj3sGc

common/workerpool.c Outdated Show resolved Hide resolved
@christian-rauch
Copy link
Collaborator

I have updated the commit accordingly. Since it could be specific to MSVC, it is better to keep the original code IMO.

Both macros are defined. But as you pointed out, this looks like a typo since all other Windows-specific sections use a different macro. I prefer to use the same macro for all OS-specific sections to avoid confusion.

@s-trinh s-trinh changed the title Clarify WIN32 usage Use _WIN32 macro everywhere Jun 23, 2024
@christian-rauch christian-rauch self-requested a review June 23, 2024 11:47
@christian-rauch christian-rauch merged commit 64654c0 into AprilRobotics:master Jun 23, 2024
39 checks passed
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