Skip to content

Update windows imports to use 'W' functions - #1773

Merged
andrewrk merged 6 commits into
ziglang:masterfrom
suirad:windows-wide-imports
Dec 13, 2018
Merged

Update windows imports to use 'W' functions#1773
andrewrk merged 6 commits into
ziglang:masterfrom
suirad:windows-wide-imports

Conversation

@suirad

@suiradsuirad commented Nov 22, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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

@suiradsuirad changed the title Update windows importsUpdate windows imports to use 'W' functionsNov 22, 2018
@suirad

suirad commented Nov 22, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@HejsilHejsil assigned Hejsil and unassigned HejsilNov 22, 2018
daurnimator
daurnimator reviewed Nov 23, 2018
View reviewed changes

@daurnimatordaurnimator left a comment

Copy link
Copy Markdown
Contributor

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.

@andrewrkandrewrk mentioned this pull request Nov 23, 2018
Closed
@andrewrk

andrewrk commented Nov 23, 2018

Copy link
Copy Markdown
Member

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

andrewrk
andrewrk requested changes Nov 23, 2018
View reviewed changes

@andrewrkandrewrk left a comment

Copy link
Copy Markdown
Member

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.

Comment threadstd/os/index.zig Outdated
Comment threadstd/os/index.zig Outdated
@andrewrk

andrewrk commented Nov 23, 2018

Copy link
Copy Markdown
Member

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

suirad commented Nov 24, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

suirad commented Nov 25, 2018

Copy link
Copy Markdown
ContributorAuthor

Im not sure why the pipeline is stopping where it does

@suirad

suirad commented Nov 30, 2018

Copy link
Copy Markdown
ContributorAuthor

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

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

Reviewers

@andrewrkandrewrkandrewrk requested changes
+1 more reviewer
@daurnimatordaurnimatordaurnimator 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