Conversation
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
_var function to use env_imp::getenvenv::_var function
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| Some(s) => s.into_string().map_err(VarError::NotUnicode), | ||
| None => Err(VarError::NotPresent), | ||
| } | ||
| env_imp::getenv(key).ok_or(VarError::NotPresent)?.into_string().map_err(VarError::NotUnicode) |
There was a problem hiding this comment.
I'm unsure why this refactoring is necessary. It looks like var_os(key) wraps around env_imp::getenv(key) and does what you're doing (or what you want to do) with match cases:
/// Doc comments omitted
#[stable(feature = "env", since = "1.0.0")]
pub fn var<K: AsRef<OsStr>>(key: K) -> Result<String, VarError> {
_var(key.as_ref())
}
fn _var(key: &OsStr) -> Result<String, VarError> {
match var_os(key) {
Some(s) => s.into_string().map_err(VarError::NotUnicode),
None => Err(VarError::NotPresent),
}
}
/// Doc comments omitted
#[must_use]
#[stable(feature = "env", since = "1.0.0")]
pub fn var_os<K: AsRef<OsStr>>(key: K) -> Option<OsString> {
_var_os(key.as_ref())
}
fn _var_os(key: &OsStr) -> Option<OsString> {
env_imp::getenv(key)
}Although, I'll admit, I'm curious on about the separate _var_os() and _var() functions here; I'm not noticing anywhere else _var_os() and _var() is used, so it makes me wonder why not just put what's inside _var_os()/_var() inside var_os()/var()?
There was a problem hiding this comment.
The split exists to minimize the size of the generic versions.
There was a problem hiding this comment.
Oh got it, that makes sense to me!
There was a problem hiding this comment.
I'm unsure why this refactoring is necessary.
Primarily to avoid: var -> _var -> -> var_os -> _var_os -> env_imp::getenv
When we can do: var -> _var -> env_imp::getenv
Replacing match with combinators because I thought it read a bit nicer
There was a problem hiding this comment.
Yea I can definitely see the layers of function indirection here; I'm wondering if the compiler would optimize this away though and inline it?
Still, I'm with @hkBst on this part:
not a fan of using
env_imp::getenvdirectly here, when this is clearly function that does a small modification to the output of var_os
| Some(s) => s.into_string().map_err(VarError::NotUnicode), | ||
| None => Err(VarError::NotPresent), | ||
| } | ||
| env_imp::getenv(key).ok_or(VarError::NotPresent)?.into_string().map_err(VarError::NotUnicode) |
There was a problem hiding this comment.
I'm not a fan of using env_imp::getenv directly here, when this is clearly function that does a small modification to the output of var_os. I suppose _var_os might be okay, but only if there was a benefit.
There was a problem hiding this comment.
Not sure why env_imp::getenv is an issue, it’s one less indirection, avoids unnecessary generic var_os calls, and _var_os seems like it should be an inner function to var_os.
|
Looks like @hkBst is on top of reviewing things already. r? hkBst |
|
Failed to set assignee to
|
|
Well, let me know when you think it's good to go and I can approve it on your behalf 🙂 |
Sure thing! @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
No description provided.