[Executors] Ensure we treat DA older than 5.9 always as DefaultActor #64800
Conversation
| // introduction of custom executors for distributed actors, must be treated as default actor, | ||
| // even if it were to declared the unowned executor property, as older compilers | ||
| // do not have the the logic to handle that case. | ||
| return true; |
There was a problem hiding this comment.
This is the core of the change; we ensure we treat distributed actors which "could not have been non-default" always as default.
| tests.test("5.7 actor, 5.9 executor property => no custom executor") { | ||
| expectCrashLater(withMessage: "Fatal error: Incorrect actor executor assumption; Expected 'MainActor' executor.") | ||
| try! await FiveSevenActor_FiveNineExecutor(actorSystem: system).test(x: 42) | ||
| } |
There was a problem hiding this comment.
This case I was unsure about, but I think this way to handle it makes sense; the entire actor has to be 5.9 to be treated like being able to have a custom executor, not just the property.
ktoso
added
concurrency
|
@swift-ci please smoke test |
|
Going to debug the linux issue 👀 |
ktoso
force-pushed
the
wip-da-olderthan-59-always-default-actor
branch
from
1187023 to
931be88
Compare
ktoso
force-pushed
the
wip-da-olderthan-59-always-default-actor
branch
from
931be88 to
3b4ac6c
Compare
|
Fixed non apple platform issues:
|
|
@swift-ci please smoke test |
|
|
||
| /// Returns a representation of this range as a string for debugging purposes. | ||
| std::string getAsString() const { | ||
| return "AvailabilityContext(" + OSVersion.getAsString() + (isAvailableAsSPI() ? ", spi" : "") + ")"; |
There was a problem hiding this comment.
Minor: run git-clang-format
There was a problem hiding this comment.
Thanks, I'll follow up separately if that's fine? That'd just be a linebreak here AFAICS.
- return "AvailabilityContext(" + OSVersion.getAsString() + (isAvailableAsSPI() ? ", spi" : "") + ")";
+ return "AvailabilityContext(" + OSVersion.getAsString() +
+ (isAvailableAsSPI() ? ", spi" : "") + ")";// The problem with clang-format is that it is not being applied consistently in this project, so applying it adds all kinds of noise and "this commit does not look like the rest of the file or method at all now..."
There was a problem hiding this comment.
Follow up is fine with me! I find the clang-format output is pretty consistent with the style of most of the files I have worked in in the project and it is documented in https://github.com/apple/swift/blob/main/docs/HowToGuides/FirstPullRequest.md as the primary way of enforcing the project code style. In particular, long lines stick out a lot - I think the majority of the codebase adheres to the 80 char limit
There was a problem hiding this comment.
I'll follow up about my specific changes, but the reality is that files are not well formatted:
22 files changed, 65 insertions(+), 65 deletions(-)
if running clang-format on files this PR touched, out of that my changes would be...
5 files changed, 22 insertions(+), 15 deletions(-)
but yeah, I'll add a git hook to git-clang-format my commits and I'll do the promised follow up now as well: #64886
This is because cross module references to a RESILIENT actor would have short circuited and returned false for such actor, while an old compiler does not have code to handle the "NON default distributed actor" paths, so it would result in compile issues in HopToActor optimization