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

gh-117891: handle undefined TARGET_OS_OSX #117892

Closed
wants to merge 1 commit into from

Conversation

jmroot
Copy link
Contributor

@jmroot jmroot commented Apr 15, 2024

This macro is not defined by older macOS SDKs, and the code in Modules/_testexternalinspection.c previously didn't distinguish between it being undefined and being defined to 0.

This macro is not defined by older macOS SDKs, and the code previously
didn't distinguish between it being undefined and being defined to 0.
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This functionally looks fine; however, as I mentioned on my review of #117887, there's another 2 related uses of TARGET_OS symbols that need to be cleaned up; I'm not sure it's better to tackle all of them in one PR, rather than multiple PRs fixing one each.

@ned-deily
Copy link
Member

ned-deily commented Apr 18, 2024

I've commented on this here #117887 (comment)

@ned-deily
Copy link
Member

Thanks again for the issue and PR. The suggested change has been rolled up and merged in PR #118073.

@ned-deily ned-deily closed this Apr 19, 2024
@jmroot jmroot deleted the testexternalinspection-fix branch April 19, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants