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

FileSystem: make all classes final to fix Sendable errors #406

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

MaxDesiatov
Copy link
Contributor

These classes can't be Sendable if they aren't final. Somehow it didn't cause CI failures before, but had impact on Swift Driver build jobs.

rdar://108040704

These classes can't be `Sendable` if they aren't final. Somehow it didn't cause CI failures before, but had impact on Swift Driver  build jobs.
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@tomerd
Copy link
Contributor

tomerd commented Apr 14, 2023

I wonder if all these need to be classes. do they carry any state?

@neonichu
Copy link
Contributor

I wonder if all these need to be classes. do they carry any state?

I think local filesystem doesn't since it is just a wrapper around other APIs, but the various virtual-ish filesystems need to.

@MaxDesiatov
Copy link
Contributor Author

InMemoryFileSystem does have its own state, which itself is a class, and it probably would be weird if assigning it to a different variable and modifying it created a copy, as underlying storage is CoW. It could probably even break some existing behavior. RerootedFileSystemView on the other hand only holds a reference to an underlying filesystem, so from that perspective it's an aggregate and could be a struct. If you'd like any relevant changes to be made, should they go to this same PR or a separate one?

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) April 14, 2023 17:42
@MaxDesiatov
Copy link
Contributor Author

Enabling auto-merge just for the sake of unblocking whatever CI job uncovered this as an error in Swift Driver.

@tomerd
Copy link
Contributor

tomerd commented Apr 14, 2023

right, InMemoryFileSystem makes sense to be a class given that it has state. the point I was trying to make is that we should audit them and make sure we only use class where we need to and change the others to structs. this PR is fine IMO

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit 5e1f556 into main Apr 15, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/final-filesystem branch April 18, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants