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

Define OSX if TARGET_OS_OSX for convenience #1658

Merged
merged 4 commits into from Nov 17, 2020

Conversation

oviano
Copy link
Contributor

@oviano oviano commented Nov 13, 2020

It's convenient when not using the CMAKE system for platform_sys.h to define OSX when TARGET_OS_OSX is defined, to avoid having to define OSX, which really shouldn't be necessary.

@codecov

This comment has been minimized.

@maxsharabayko
Copy link
Collaborator

A TODO note: consider #1243.

@oviano
Copy link
Contributor Author

oviano commented Nov 13, 2020

A TODO note: consider #1243.

I just had a look at that, the suggestion seems to be to use the CMAKE_SYSTEM_NAME variable throughout. That strikes me as inferior to using the standard Apple definitions that are defined in TargetConditionals.h and which most of the code is already including via platform_sys.h.

Best solution, IMO would be to:

  1. make sure TargetConditionals.h is always included, via platform_sys.h. This is mostly the case, but I noticed as part of my other PR that some files don't include platform_sys.h.

  2. use TARGET_OS_OSX along with TARGET_OS_IOS and TARGET_OS_TV. This is standard in Apple world.

Otherwise you've got to basically define "CMAKE_SYSTEM_NAME" if you are using this code but not using the CMAKE system.

If that is not a valid use case then fair enough, bit it seems a little back-to-front to me....

@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Nov 13, 2020
@maxsharabayko maxsharabayko added [build] Area: Changes in build files Type: Maintenance Work required to maintain or clean up the code labels Nov 13, 2020
@oviano
Copy link
Contributor Author

oviano commented Nov 13, 2020

Ok, I updated the PR with what in my view is the proper solution, but it's entirely up to you guys if you take it 👍

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Nov 15, 2020

Maybe a single TARGET_OS_MAC instead of the three TARGET_OS_IOS, TARGE_OS_OSX and TARGET_OS_TV would work... 🤔
Target Conditionals

  +----------------------------------------------------------------+
  |                TARGET_OS_MAC                                   |
  | +---+  +-----------------------------------------------------+ |
  | |   |  |          TARGET_OS_IPHONE                           | |
  | |OSX|  | +-----+ +----+ +-------+ +--------+ +-------------+ | |
  | |   |  | | IOS | | TV | | WATCH | | BRIDGE | | MACCATALYST | | |
  | |   |  | +-----+ +----+ +-------+ +--------+ +-------------+ | |
  | +---+  +-----------------------------------------------------+ |
  +----------------------------------------------------------------+

@oviano
Copy link
Contributor Author

oviano commented Nov 16, 2020

That makes sense, I've updated the PR.

@maxsharabayko maxsharabayko linked an issue Nov 16, 2020 that may be closed by this pull request
@maxsharabayko
Copy link
Collaborator

Sorry, one more suggestion, given that we are looking at these definitions. 🙂

Comparing TARGET_OS_MAC with 1 can be improved in the following way.

For example:

#elif (TARGET_OS_MAC == 1)

can be

#elif TARGET_OS_MAC

Another example condition:

#elif defined(BSD) || TARGET_OS_MAC

@maxsharabayko maxsharabayko self-assigned this Nov 16, 2020
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core and removed [build] Area: Changes in build files labels Nov 17, 2020
@maxsharabayko maxsharabayko merged commit b0a9d4d into Haivision:master Nov 17, 2020
@maxsharabayko
Copy link
Collaborator

Thank you for your input, @oviano!

@oviano oviano deleted the define-osx-if-undefined branch November 17, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use DARWIN instead of TARGET_OS_TV and others
2 participants