Skip to content

Always use FS representation of String when calling FS syscalls #1540

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

Merged
merged 1 commit into from
May 3, 2018

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Apr 29, 2018

  • Update calls to stat(), lstat(), access(), link(), unlink(),
    symlink(), readlink(), rename(), chmod(), mkdir() and chdir().

  • Fix deallocating the fs representation in FileManager._lstatFile(withPath:)
    and URL.withUnsafeFileSystemRepresentation().

  • Add FileManager._copySymlink(atPath:toPath:) to copy a symlink
    without the roundtrip via String.

- Update calls to stat(), lstat(), access(), link(), unlink(),
  symlink(), readlink(), rename(), chmod(),  mkdir() and chdir().

- Fix deallocating the fs representation in FileManager._lstatFile(withPath:)
  and URL.withUnsafeFileSystemRepresentation().

- Add FileManager._copySymlink(atPath:toPath:) to copy a symlink
  without the roundtrip via String.
@spevans
Copy link
Contributor Author

spevans commented Apr 29, 2018

@swift-ci please test

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented May 1, 2018

@swift-ci please test

@spevans spevans requested a review from parkera May 2, 2018 17:49
@parkera parkera requested a review from kperryua May 2, 2018 20:11
Copy link
Contributor

@kperryua kperryua left a comment

Choose a reason for hiding this comment

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

This all seems fine, but I wonder if it suggests that there's a bigger problem. Should Swift be allowing code to pass String to these file system APIs at all? Is it documented anywhere what normalization doing so results in?

In Objective-C, you obviously can't pass an NSString to lstat directly, so you're forced to at least think about how to convert to a C string. In Swift, there's nothing to make you think, which makes it easier for others to get this "wrong".

@spevans
Copy link
Contributor Author

spevans commented May 3, 2018

@swift-ci please test and merge

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