Skip to content

[Concurrency] Fix the missing builtin guards for DiscardingTaskGroup #70837

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

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jan 11, 2024

The PR #70537 adjusted the way we create tasks for the discarding group by using a new builtin -- this builtin must be protected with a #if $BuiltinCreateAsyncDiscardingTaskInGroup as otherwise we can get issues like module 'Builtin' has no member named 'createAsyncDiscardingTaskInGroup' when an old compiler without this builtin is used.

FYI @kateinoigakukun @MaxDesiatov

Resolves rdar://120801949

@ktoso ktoso force-pushed the wip-guard-builtin-BuiltinCreateAsyncDiscardingTaskInGroup branch from 692f294 to f203571 Compare January 11, 2024 04:12
// correct 'createAsyncDiscardingTaskInGroup' when available (and a recent
// enough compiler is used).
_ = Builtin.createAsyncTaskInGroup(flags, _group, operation)
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the right dance to do here -- the comment attempts to explain what we're doing here -- See the PR which introduced this change here: https://github.com/apple/swift/pull/70537/files#diff-9897ab704fe1b9e57499ec46e8837c683573998d299ea3cbfb64d29c4a34544aL567

So we're using what "used to work" on other platforms and old compilers, and when available, we use the "right thing".

Looks right @slavapestov @kavon ?

@ktoso ktoso requested a review from kateinoigakukun January 11, 2024 04:14
@ktoso
Copy link
Contributor Author

ktoso commented Jan 11, 2024

@swift-ci please smoke test

@ktoso ktoso enabled auto-merge (squash) January 11, 2024 04:19
@kateinoigakukun kateinoigakukun enabled auto-merge (squash) January 11, 2024 04:31
Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

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

Ah, good catch, Thanks!

@ktoso
Copy link
Contributor Author

ktoso commented Jan 11, 2024

Luckily it got caught in testing before it causes more trouble 😅
My bad that I missed it during review, thanks for the fix @kateinoigakukun !

@kateinoigakukun kateinoigakukun merged commit ef80d77 into swiftlang:main Jan 11, 2024
@ktoso ktoso deleted the wip-guard-builtin-BuiltinCreateAsyncDiscardingTaskInGroup branch January 11, 2024 07:07
tshortli added a commit to tshortli/swift that referenced this pull request Jan 11, 2024
Catfish-Man pushed a commit to Catfish-Man/swift that referenced this pull request Jan 19, 2024
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.

3 participants