Prefer unwrap_or_else to unwrap_or in case of function calls/allocations #55014
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/bootstrap/compile.rs
Outdated
| @@ -544,7 +544,7 @@ pub fn rustc_cargo_env(builder: &Builder, cargo: &mut Command) { | |||
| .env("CFG_PREFIX", builder.config.prefix.clone().unwrap_or_default()) | |||
| .env("CFG_CODEGEN_BACKENDS_DIR", &builder.config.rust_codegen_backends_dir); | |||
|
|
|||
| let libdir_relative = builder.config.libdir_relative().unwrap_or(Path::new("lib")); | |||
| let libdir_relative = builder.config.libdir_relative().unwrap_or_else(|| Path::new("lib")); | |||
There was a problem hiding this comment.
Hm, ideally, using the _else variant could be a great indicator whether the computation is actually expensive. This change, for example, is not necessary: It is just a cast.
I'm fine with changing all function calls to _else for now, though.
There was a problem hiding this comment.
Yeah most of these _else are line noise with no positive impact. Many wrap functions like String::new, Vec::new, tuple struct constructors, etc. that are literally free.
There was a problem hiding this comment.
@rkruppe well, most are to_string(), which are not free.
There was a problem hiding this comment.
Fair, I haven't actually counted, but I did see a significant number of them.
There was a problem hiding this comment.
@rkruppe you are 100% right about Vec/String::new, though; I knew they didn't allocate, but I didn't notice they had become const fn.
ljedrz
force-pushed
the
lazyboye_unwraps
branch
from
0cbd7fe to
28945f8
Compare
|
Comments addressed; I replaced calls to |
|
☔ The latest upstream changes (presumably #54941) made this pull request unmergeable. Please resolve the merge conflicts. |
ljedrz
force-pushed
the
lazyboye_unwraps
branch
from
28945f8 to
2aee076
Compare
|
@matthewjasper rebased. |
|
☔ The latest upstream changes (presumably #55171) made this pull request unmergeable. Please resolve the merge conflicts. |
ljedrz
force-pushed
the
lazyboye_unwraps
branch
from
2aee076 to
28b52ea
Compare
|
Rebased. |
|
☔ The latest upstream changes (presumably #54976) made this pull request unmergeable. Please resolve the merge conflicts. |
ljedrz
force-pushed
the
lazyboye_unwraps
branch
from
28b52ea to
d28aed6
Compare
|
Re(eee)based. @kennytm these are trivial changes good for a rollup if they can be squeezed in; I won't be able to rebase for the next few days. |
|
@bors r+ |
|
📌 Commit d28aed6 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit d28aed6 with merge 398c664f3a4b3cfb83992c05604f87266a029e38... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
Prefer unwrap_or_else to unwrap_or in case of function calls/allocations The contents of `unwrap_or` are evaluated eagerly, so it's not a good pick in case of function calls and allocations. This PR also changes a few `unwrap_or`s with `unwrap_or_default`. An added bonus is that in some cases this change also reveals if the object it's called on is an `Option` or a `Result` (based on whether the closure takes an argument).
|
☀️ Test successful - status-appveyor, status-travis |
The contents of
unwrap_orare evaluated eagerly, so it's not a good pick in case of function calls and allocations. This PR also changes a fewunwrap_ors withunwrap_or_default.An added bonus is that in some cases this change also reveals if the object it's called on is an
Optionor aResult(based on whether the closure takes an argument).