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

Remove exclusivity support for Swift 3 mode. #19849

Merged
merged 3 commits into from
Oct 15, 2018
Merged

Remove exclusivity support for Swift 3 mode. #19849

merged 3 commits into from
Oct 15, 2018

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Oct 12, 2018

Remove the compiler support for exclusivity warnings.

Leave runtime support for exclusivity warnings in non-release builds
only for unit testing convenience.

Remove a test case that checked the warning log output.

Modify test cases that relied on successful compilation in the
presence of exclusivity violations.

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test source compatibility.

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

Fixes rdar://problem/45146046 Remaining -swift-version 3 tests for exclusivity.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - abd0e43faa1e319bd18a70ad586febb9fe583999

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test source compatibility.

@slavapestov
Copy link
Contributor

Thanks!

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - abd0e43faa1e319bd18a70ad586febb9fe583999

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - abd0e43faa1e319bd18a70ad586febb9fe583999

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Oct 12, 2018

@swift-ci test source compatibility.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - fa3924199a5143dfd8443038330e88e87b7c186c

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - fa3924199a5143dfd8443038330e88e87b7c186c

Copy link
Contributor

@devincoughlin devincoughlin left a comment

Choose a reason for hiding this comment

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

Looks good to me! Some comments in line.

@@ -13,7 +13,7 @@ public func takesInout(_ i: inout Int) {
func takeClosure(_: ()->Int) {}

// Helper taking a basic, no-escape closure.
func takeClosureAndInout(_: inout Int, _: ()->Int) {}
func takeClosureAndInout(_: inout Int, _: @escaping ()->Int) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the comment here is stale since the closure is now escaping.

@@ -570,18 +563,16 @@ static void diagnoseExclusivityViolation(const ConflictingAccess &Violation,
}

auto D =
diagnose(Ctx, MainAccess.getAccessLoc().getSourceLoc(), DiagnosticID,
diagnose(Ctx, MainAccess.getAccessLoc().getSourceLoc(),
diag::exclusivity_access_required,
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we're only using the non-warning variants it should be safe to delete exclusivity_access_required_warn and f exclusivity_access_required_unknown_decl_warn rom DiagnosticsSIL.def

@@ -997,8 +997,10 @@ enum class ExclusivityFlags : uintptr_t {
// Read or Modify).
ActionMask = 0x1,

// Downgrade exclusivity failures to a warning.
#ifndef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

One possibility to remove the need to expose this in the ABI header is to separate out the unit tests that abort into a separate testing tool (similar to swift-ide-test) and turn the unit tests into file check tests.

Remove the compiler support for exclusivity warnings.

Leave runtime support for exclusivity warnings in non-release builds
only for unit testing convenience.

Remove a test case that checked the warning log output.

Modify test cases that relied on successful compilation in the
presence of exclusivity violations.

Fixes: <rdar://problem/45146046> Remaining -swift-version 3 tests for exclusivity
@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2018

@devincoughlin @rjmccall I removed the remaining runtime support for warnings and moved the runtime unit tests into a library.

@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2018

@swift-ci test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - aeda25997c7c5898df89c65bf35f9334f3b22163

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - aeda25997c7c5898df89c65bf35f9334f3b22163

@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2018

@swift-ci test linux platform.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 2f689f4857e913133519c20e93ffdbef2a370680

Create a new RuntimeUnittest library alongside the other stdlib unit
tests so we can write C++ runtime unit tests callable from lit.

Move runtime exclusivity tests into the stdlib unittest library and
create lit tests so we can verify that the runtime crashes with an
error message.
@atrick
Copy link
Contributor Author

atrick commented Oct 13, 2018

@swift-ci test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 2f689f4857e913133519c20e93ffdbef2a370680

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 2f689f4857e913133519c20e93ffdbef2a370680

@atrick atrick merged commit e3a75de into swiftlang:master Oct 15, 2018
@atrick atrick deleted the remove-swift3-exclusivity branch October 16, 2018 16:17
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.

4 participants