Skip to content

Follow up for changes requested in #1940 #1941

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
Merged
merged 1 commit into from
Feb 24, 2019
Merged

Follow up for changes requested in #1940 #1941

merged 1 commit into from
Feb 24, 2019

Conversation

compnerd
Copy link
Member

No description provided.

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

CC: @gwynne @millenomi

gwynne
gwynne approved these changes Feb 23, 2019
View reviewed changes
Copy link
Contributor

@gwynne gwynne left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really delighted by having Array<> right next to UnsafeMutableBufferPointer<> when either one could serve the purpose for both cases, but other than that I love it 🙂 @compnerd has mentioned possibly normalizing this kind of usage across the Windows implementations at a later time, which is fine by me. I am curious which type of buffer is more in keeping with Foundation's desired implementation style, however. @millenomi ?

spevans
spevans reviewed Feb 23, 2019
View reviewed changes
gwynne
gwynne suggested changes Feb 23, 2019
View reviewed changes
Copy link
Contributor

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Looks good, other than @spevans' comment about the trailing /

@compnerd compnerd mentioned this pull request Feb 23, 2019
Merged
@compnerd
Copy link
Member Author

@gwynne - I would like to get @millenomi to okay this as well since this has a subtle difference in behaviour (we will never go down the path of the env var check).

@gwynne
Copy link
Contributor

gwynne commented Feb 24, 2019

@compnerd 👍 Definitely want @millenomi 's sign-off on this one.

gwynne
gwynne previously approved these changes Feb 24, 2019
View reviewed changes
@gwynne gwynne requested a review from millenomi February 24, 2019 02:34
@gwynne gwynne dismissed their stale review February 24, 2019 02:37

Awaiting further feedback

millenomi
millenomi reviewed Feb 24, 2019
View reviewed changes
gwynne
gwynne suggested changes Feb 24, 2019
View reviewed changes
Copy link
Contributor

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Need error check on second call to GetTempPathW() to avoid TOCTOU race

@compnerd
Foundation: dynamically allocate temporary path
7ac5d69 
Query the desired buffer size and allocate an appropriate buffer for the
temporary path query.  This should prevent a truncated path from being
given.
@compnerd compnerd force-pushed the temporary-temporaries branch from 8cf5bae to 7ac5d69 Compare February 24, 2019 04:29
@millenomi
Copy link
Contributor

@swift-ci please test and merge

1 similar comment
@millenomi
Copy link
Contributor

@swift-ci please test and merge

@gwynne
Copy link
Contributor

gwynne commented Feb 24, 2019

Holding off until I can push an additional correction to the Darwin error handling.

@gwynne gwynne self-requested a review February 24, 2019 06:39
@gwynne gwynne self-requested a review February 24, 2019 06:39
@gwynne
Copy link
Contributor

gwynne commented Feb 24, 2019

(Sorry for the spam about reviews, I'm still learning what some of these buttons do 😓)

gwynne
gwynne approved these changes Feb 24, 2019
View reviewed changes
Copy link
Contributor

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Will do a separate PR

@swift-ci swift-ci merged commit 6f019e2 into swiftlang:master Feb 24, 2019
@compnerd compnerd deleted the temporary-temporaries branch February 24, 2019 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spevans spevans spevans left review comments

@millenomi millenomi millenomi approved these changes

@gwynne gwynne gwynne approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@compnerd @gwynne @millenomi @spevans @swift-ci