Skip to content

Update windows imports to use 'W' functions #1773

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
andrewrk merged 6 commits into from
Dec 13, 2018
Merged

Update windows imports to use 'W' functions #1773

andrewrk merged 6 commits into from
Dec 13, 2018

Conversation

@suirad
Copy link
Contributor

@suirad suirad commented Nov 22, 2018 •
edited
Loading

  • Change FreeEnvironmentStringsA to FreeEnvironmentStringsW
  • Change GetEnvironmentVariableA to GetEnvironmentVariableW
  • Change GetEnvironmentStringsA to GetEnvironmentStringsW
  • Updated os.getEnvMap to use the stack for environment keys and values with a length under 50 to be built on the stack; anything larger is allocated on the heap.

Addresses #534

@suirad suirad changed the title Update windows imports Update windows imports to use 'W' functions Nov 22, 2018
@suirad
Copy link
Contributor Author

suirad commented Nov 22, 2018

Are new tests desired for these pulls? I made some locally, but wasn't sure if they were wanted.

@Hejsil Hejsil assigned Hejsil and unassigned Hejsil Nov 22, 2018
daurnimator
daurnimator reviewed Nov 23, 2018
View reviewed changes
Copy link
Contributor

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

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

I have 0 context here, but are you aware that windows doesn't use well-formed UTF16? This ill-formed utf16 is not safely convertible to utf8. When converting you have to use "WTF-8" instead.

@andrewrk andrewrk mentioned this pull request Nov 23, 2018
Closed
@andrewrk
Copy link
Member

andrewrk commented Nov 23, 2018

@daurnimator thanks - I filed #1774 in response to your comment.

andrewrk
andrewrk requested changes Nov 23, 2018
View reviewed changes
Copy link
Member

@andrewrk andrewrk 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 - an optional improvement and a minor documentation addition and then it's ready for merge.

@andrewrk
Copy link
Member

andrewrk commented Nov 23, 2018

Are new tests desired for these pulls? I made some locally, but wasn't sure if they were wanted.

Tests would be much appreciated

@suirad
Copy link
Contributor Author

suirad commented Nov 24, 2018 •
edited
Loading

Code has been updated for review.

  • Updated os.getEnvMap to use the stack for environment keys and values with a length under 50 to be built on the stack; anything larger is allocated on the heap.

@suirad
Copy link
Contributor Author

suirad commented Nov 25, 2018

Im not sure why the pipeline is stopping where it does

@suirad
Copy link
Contributor Author

suirad commented Nov 30, 2018

@andrewrk which env variables should i have the tests look for?

@andrewrk andrewrk merged commit cf266ff into ziglang:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewrk andrewrk andrewrk requested changes

+1 more reviewer

@daurnimator daurnimator daurnimator left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@suirad @andrewrk @daurnimator @Hejsil