Skip to content

[Concurrency][SE-review update] Task names update #80984

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Concurrency][SE-review update] Task names update #80984

wants to merge 2 commits into from
+399 −384

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 22, 2025

No description provided.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

ktoso
ktoso commented Apr 22, 2025
View reviewed changes
stdlib/public/Concurrency/Task+init.swift.gyb

import Swift

// ==== Task.init ------------------------------------------------
Copy link
Contributor Author

@ktoso ktoso Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Continuing the quest to stop writing almost the same method 12 times 😉

  • init (3)
    • init
    • SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY
    • $Embedded version
  • init + throwing (3)
    • ...
  • detached (3)
    • ...
  • detached + throwing (3)
    • ...

So we're saving some repetition here... This should eventually also consume the Task.swift and Task+TaskExecutor.swift initializers, but I'm taking it one step at a time, to not cause unnecessary risk for this change.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

Linux failure seems unrelated

error: link command failed with exit code 1 (use -v to see invocation)
Sources/plutil/CMakeFiles/plutil.dir/PLUContext_KeyPaths.swift.o:main.swift.o:function $sSS6plutilE19escapedKeyPathSplitSaySSGyF: error: undefined reference to '$sSy12RegexBuilderSs11SubSequenceRtzrlE5split9separator9maxSplits25omittingEmptySubsequencesSaySsGSS_SiSbtF'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@ktoso ktoso changed the title task names review update [Concurrency][SE-review update] Task names update Apr 22, 2025
@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

Windows too hm


TimeZone_ICU.swift.obj : error LNK2001: unresolved external symbol __imp_$sSS17_StringProcessing14RegexComponent0C7BuilderMc

bin\FoundationInternationalization.dll : fatal error LNK1120: 2 unresolved externals

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

ktoso
ktoso commented Apr 22, 2025
View reviewed changes
stdlib/public/Concurrency/TaskGroup+addTask.swift.gyb Outdated
@@ -31,7 +31,7 @@ import Swift
% 'ThrowingDiscardingTaskGroup'
% ],
% [
% '@available(SwiftStdlib 6.2, *)',
% '@available(SwiftStdlib 5.1, *)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

small win, but it's nice to fix just a single line rather than hunt around 8 locations where to fix it

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please test source compatibility

@ktoso
Copy link
Contributor Author

ktoso commented Apr 22, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 23, 2025

Well, this turned out quite more complex:

  • 16 declarations for all the task groups task groups
    • some of them, 5.1
    • some of them 5.9 (because DiscardingTaskGroup)
    • some of them 6.0 (because any API accepting TaskExecutor)
  • 24 declarations of Task.init because of init/detached/throwing/non-throwing/startSynchronously and the various "not available in embedded" etc...
    • any Task.init with a TaskExecutor is 6.0
    • baseline are 5.1
    • 🤔 There's a silly 3x here thanks to $Embedded and SWIFT_STDLIB_TASK_TO_THREAD_MODEL_CONCURRENCY, maybe we could simplify the number of redeclarations here a bit... at least so that it's a 2x and not a 3x heh. I'll think more about it.
  • 10 declarations for startSynchronously
    • 8 for the various task groups
    • 2 for Task.init -- one throwing and one not throwing

That's, 50 declarations in total, and I just noticed we didn't do startSynchronously + detached, so that's 2 more.

@ktoso ktoso force-pushed the task-names-update branch from 7c7607b to 16119e5 Compare April 23, 2025 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etcwilde etcwilde Awaiting requested review from etcwilde etcwilde is a code owner

@compnerd compnerd Awaiting requested review from compnerd compnerd is a code owner

@edymtt edymtt Awaiting requested review from edymtt edymtt is a code owner

@justice-adams-apple justice-adams-apple Awaiting requested review from justice-adams-apple justice-adams-apple is a code owner

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

Successfully merging this pull request may close these issues.
1 participant
@ktoso