Skip to content

[stdlib] Change os.env functions to use StringSlice #4305

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

Closed
wants to merge 4 commits into from

Conversation

martinvuyk
Copy link
Contributor

Change os.env functions to use StringSlice

@martinvuyk martinvuyk requested a review from a team as a code owner April 6, 2025 20:38
@owenhilyard
Copy link
Contributor

env args are mutable, so it's probably best that either the runtime makes a copy at the start of the program or that we do make a copy here due to hazards around C libraries modifying them.

@soraros
Copy link
Contributor

soraros commented Apr 7, 2025

@owenhilyard The setenv (C version makes a internal copy unlike putenv) and unsetenv take a StringSlice in Mojo land, and the getenv performs a copy on return. I don't see any issues with this approach.

@soraros
Copy link
Contributor

soraros commented Apr 7, 2025

Actually, do we want to remove unsafe_cstr_ptr (different from unsafe_c_char_ptr which is a purely ABI thing)? We would lose an important API contract, namely, null termination, even if only from a documentation perspective.

@martinvuyk
Copy link
Contributor Author

Actually, do we want to remove unsafe_cstr_ptr (different from unsafe_c_char_ptr which is a purely ABI thing)? We would lose an important API contract, namely, null termination, even if only from a documentation perspective.

I've wanted to deprecate unsafe_cstr_ptr for a long time (remember #3638 ?)

@owenhilyard
Copy link
Contributor

@owenhilyard The setenv (C version makes a internal copy unlike putenv) and unsetenv take a StringSlice in Mojo land, and the getenv performs a copy on return. I don't see any issues with this approach.

That's probably fine so long as we provide locking on the Mojo side, since all of the C library functions are thread unsafe.

martinvuyk and others added 4 commits April 15, 2025 11:39
@martinvuyk martinvuyk force-pushed the change-os-env-stringslice branch from 4c694f2 to d0f7b8d Compare April 15, 2025 15:39
@martinvuyk martinvuyk closed this May 17, 2025
@martinvuyk martinvuyk deleted the change-os-env-stringslice branch May 17, 2025 14:17
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.

4 participants