Skip to content

[Parse] Allow #if to guard switch case clauses #9457

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 28, 2017
Merged

[Parse] Allow #if to guard switch case clauses #9457

merged 2 commits into from
Jun 28, 2017

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented May 10, 2017 •
edited
Loading

Resolves: SR-4196, SR-2

evolution discussion: https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170508/036568.html

Depends on #9413: Only the last commit is relevant to this PR. merged

@rintaro
Copy link
Member Author

rintaro commented May 10, 2017

@swift-ci Please smoke test

@rintaro rintaro force-pushed the parse-ifconfig-switchcase branch 2 times, most recently from 3ca980b to 16cf0cf Compare May 10, 2017 08:25
@rintaro
Copy link
Member Author

rintaro commented May 10, 2017

@swift-ci Please smoke test

@rintaro rintaro force-pushed the parse-ifconfig-switchcase branch from 16cf0cf to 3d8dcc1 Compare May 15, 2017 16:59
@rintaro rintaro changed the title [WIP][Parse] Allow #if to guard switch case clauses [Parse] Allow #if to guard switch case clauses May 15, 2017
@rintaro
Copy link
Member Author

rintaro commented May 15, 2017

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor

Can you add at least one execution test, so we can see that nothing down the pipeline gets tripped up by this?

@jrose-apple jrose-apple requested a review from CodaFi May 15, 2017 17:13
@CodaFi
Copy link
Contributor

CodaFi commented May 15, 2017

@jrose-apple can we get #7955 merged before I review this. It may require some rebasing but I can't tell because of the chain of dependencies here

@rintaro rintaro force-pushed the parse-ifconfig-switchcase branch 5 times, most recently from da95bb1 to 050816b Compare May 17, 2017 04:47
@rintaro rintaro force-pushed the parse-ifconfig-switchcase branch from 050816b to 5d478bd Compare June 17, 2017 01:04
@rintaro
Copy link
Member Author

rintaro commented Jun 17, 2017

Rebased on master
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Jun 17, 2017

@swift-ci Please smoke test

@garricn garricn mentioned this pull request Jun 17, 2017
Closed
@rintaro
Copy link
Member Author

rintaro commented Jun 21, 2017

@CodaFi Could you review this?

@CodaFi
Copy link
Contributor

CodaFi commented Jun 21, 2017

One last thing before I merge this: Can you add something to the existing tests that uses nested configuration blocks?

@rintaro
Copy link
Member Author

rintaro commented Jun 22, 2017

Added test case in executable test.
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Jun 28, 2017

@CodaFi Could you merge this?
Also, do you think it's possible to get this into 4.0 branch?

@CodaFi
Copy link
Contributor

CodaFi commented Jun 28, 2017 •
edited
Loading

⛵️

You can cherry pick this to 4.0. I will nominate internally.

@CodaFi CodaFi merged commit d07eb71 into swiftlang:master Jun 28, 2017
@CodaFi
Copy link
Contributor

CodaFi commented Jun 28, 2017

Sorry to contradict myself so directly: If it were just this patch we would consider it, but because this feature depends on the earlier refactoring passes this is going to involve cherry-picking too much new code into 4.0 branch.

Sorry, but it's probably not a good idea right now to take this.

@rintaro
Copy link
Member Author

rintaro commented Jun 28, 2017

@CodaFi Thanks for merging.
Yeah, agreed. I will wait for 4.1.

@rintaro rintaro deleted the parse-ifconfig-switchcase branch June 28, 2017 17:28
@rintaro rintaro mentioned this pull request Jul 16, 2017
Merged
@eerie444 eerie444 mentioned this pull request Oct 31, 2019
Closed
@rintaro rintaro mannequin mentioned this pull request May 10, 2017
Closed
@AnthonyLatsis AnthonyLatsis mentioned this pull request Nov 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodaFi CodaFi CodaFi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rintaro @jrose-apple @CodaFi