std::lib::os: add get_home_dir() and get_config_dir() #1010
Conversation
lib/std/os/env.c3
Outdated
| **/ | ||
| fn String! get_config_dir(Allocator* using = mem::heap()) | ||
| { | ||
| $if env::WIN32: |
There was a problem hiding this comment.
No, I meant to use Path to append paths together. I think your code is also incorrect, forgetting to add '/' in some cases. Path ensures valid paths and can concatenate into new paths.
There was a problem hiding this comment.
I have already fixed the missing / for Linux&co.
I dont think Path is required as the concatenation occurs when it is known that this is a non Windows code path.
lib/std/os/env.c3
Outdated
| /** | ||
| * Returns the current user's home directory. | ||
| **/ | ||
| fn String! get_home_dir() |
There was a problem hiding this comment.
What is the ownership and allocation origin of String here?
There was a problem hiding this comment.
Good question. I was wondering the same about get_var(). I think an allocator may be required for those functions.
There was a problem hiding this comment.
get_var() is grabbing it from the os which then might change it, but there is no need to deallocate it.
There was a problem hiding this comment.
get_var() is grabbing it from the os which then might change it, but there is no need to deallocate it.
It currently does for non-WIndows os'es but with WIN32 you need to allocate it.
Will add the allocator for WIN32.
There was a problem hiding this comment.
Best is probably to make the non-Win32 also allocate.
lib/std/os/env.c3
Outdated
| str.init(s.len + DIR.len, using); | ||
| str.append(s); | ||
| str.append(DIR); | ||
| return str.as_str(); |
There was a problem hiding this comment.
This is returning string as a String, but if you try to release this String then that will fail, because the string is part of a DString. as_str() is getting the view. Maybe we should rename all of these .str_view() to clarify it.
There was a problem hiding this comment.
It makes sense as there is already .array_view(). Maybe this should be done before this PR?
lib/std/os/env.c3
Outdated
| buffer.reserve(n - N); | ||
| get_env_win32(name, buffer); | ||
| } | ||
| return buffer.str_view()[:n]; |
There was a problem hiding this comment.
Still buggy, it will not be possible to release this string. You want to allocate the buffer with a simple tmalloc, create a string from it and copy it with using.
There was a problem hiding this comment.
Then:
return buffer.str_view()[:n].copy(using);
lib/std/os/env.c3
Outdated
| str.init(s.len + DIR.len, using); | ||
| str.append(s); | ||
| str.append(DIR); | ||
| return str.str_view(); |
There was a problem hiding this comment.
Returns the view of a dstring, so cannot be released.
lib/std/os/win32/process.c3
Outdated
| @@ -83,7 +83,15 @@ extern fn Win32_BOOL getOverlappedResult( | |||
| Win32_LPDWORD lpNumberOfBytesTransferred, | |||
| Win32_BOOL bWait | |||
| ) @extern("GetOverlappedResult"); | |||
|
|
|||
| extern fn Win32_DWORD getEnvironmentVariable( | |||
There was a problem hiding this comment.
Rename: win32_GetEnvironmentVariableA
lib/std/os/win32/process.c3
Outdated
| extern fn Win32_BOOL setEnvironmentVariable( | ||
| Win32_LPCSTR lpName, | ||
| Win32_LPCSTR lpBuffer | ||
| ) @extern("SetEnvironmentVariableA"); |
There was a problem hiding this comment.
rename to win32_SetEnvironmentVariableA
lib/std/os/win32/process.c3
Outdated
| Win32_LPCSTR lpName, | ||
| Win32_LPSTR lpBuffer, | ||
| Win32_DWORD nSize | ||
| ) @extern("GetEnvironmentVariableA"); |
There was a problem hiding this comment.
Don't use the A functions, since they are not UTF8 safe. Use the W variants and do the string conversion.
There was a problem hiding this comment.
I did try with GetEnvironmentVariable but then it couldnt be found.
|
Let me see if I can make a version of this. |
Go for it :). |
|
set / get / clear env should probably be impoved by returning an optional on error. |
Signed-off-by: Pierre Curto <[email protected]>
Signed-off-by: Pierre Curto <[email protected]>
|
Thanks for this and sorry for the long back and forth. |
|
No not at all. |
|
I think it resulted in good improvements for the stdlib. |
No description provided.