Skip to content

[stdlib] replace uses of assert in the Concurrency module #40116

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
Nov 11, 2021
Merged

[stdlib] replace uses of assert in the Concurrency module #40116

merged 2 commits into from
Nov 11, 2021

Conversation

glessard
Copy link
Contributor

@glessard glessard commented Nov 10, 2021 •
edited
Loading

  • assert was still detectable in released dylibs of libswift_Concurrency, costing execution time and code size
  • defined a local equivalent to _internalInvariant that will compile to nothing in release builds

Solves rdar://85258838

@glessard
[stdlib] replace uses of assert in the Concurrency module
eb63484 
- we define a local equialent to `_internalInvariant` that will compile to nothing in release builds
- `assert` was still detectable in release builds, presumably costing execution time
@glessard glessard requested a review from lorentey November 10, 2021 02:05
@glessard
Copy link
Contributor Author

@swift-ci please test

@glessard glessard marked this pull request as draft November 10, 2021 02:12
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - eb63484

@glessard
[test] disambiguate _internalInvariant
71f2d8e 
This test disables access controls, and it mistakenly used the Concurrency module's version of `_internalInvarant` instead of the Standard Library's version.
@glessard
Copy link
Contributor Author

@swift-ci please test

@glessard glessard requested a review from phausler November 10, 2021 19:05
phausler
phausler approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

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

This looks good to me, obviously the better solution would be to make Deque part of the standard library, but this seems like a reasonable step.

@glessard
Copy link
Contributor Author

@swift-ci please test Windows platform

@glessard glessard marked this pull request as ready for review November 11, 2021 16:50
@glessard glessard merged commit 8cc0760 into swiftlang:main Nov 11, 2021
@glessard glessard deleted the concurrency-assertions branch November 11, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phausler phausler phausler approved these changes

@lorentey lorentey Awaiting requested review from lorentey

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
@glessard @swift-ci @phausler