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

Adding macOS alias for conditional compilation blocks #3066

Closed
wants to merge 1 commit into from
Closed

Adding macOS alias for conditional compilation blocks #3066

wants to merge 1 commit into from

Conversation

erica
Copy link
Contributor

@erica erica commented Jun 19, 2016

Alias OSX to macOS for conditional compilation blocks os() test

Starting in Sierra, Apple's Mac-based OS (OS X) will be renamed "macOS".
The #if os(OSX) test should be aliased to #if os(macOS).

With any luck, I've managed to do this right..

Resolved bug number: (SR-1823)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@karwa
Copy link
Contributor

karwa commented Jun 19, 2016

Maybe add a test?

test/Parse/ConditionalCompilation/x64OSXTarget.swift is where it lives.

@jakepetroules
Copy link
Contributor

👍

@erica
Copy link
Contributor Author

erica commented Jun 20, 2016

I duplicated test/Parse/ConditionalCompilation/x64OSXTarget.swift and updated the condition from OSX to macOS

@gribozavr
Copy link
Contributor

Why not add to the existing test file? (Every test has a non-zero startup cost, adding to existing tests is cheaper.)

@erica
Copy link
Contributor Author

erica commented Jun 20, 2016

@gribozavr because I'm unsure of the protocol and this is my first (technically my second, but the first time just added a line of text) time doing a code submission

@gribozavr
Copy link
Contributor

It would be best to add the tests to the existing file.

@erica
Copy link
Contributor Author

erica commented Jun 20, 2016

@gribozavr And done

@gribozavr
Copy link
Contributor

@swift-ci Please test

@gribozavr
Copy link
Contributor

@erica Please take a look at the test issues.

@jrose-apple
Copy link
Contributor

Unfortunately, addPlatformConditionValue doesn't support multiple values for the same condition. (It should probably assert that.) I think we need to canonicalize to a particular value before checking it.

If you're busy with other things I can take this on from your starting point.

@rintaro
Copy link
Member

rintaro commented Jul 1, 2016

@jrose-apple
LangOptions::PlatformConditionValues does contain multiple values.
I added one commit on @erica 's change: master...rintaro:SR-1823
It seems to work. How do you think about this?

My concern about my patch is that the primary value is ambiguous.
Since this values is used in lib/Serialization/SerializedModuleLoader.cpp
I'm not sure supporting multiple value is right thing.

@jrose-apple
Copy link
Contributor

Right, we only expect one value. I admit I didn't think of changing the matcher to search the whole list instead of just getting the first one, but I think we should keep it unique for now and just canonicalize.

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.

6 participants