Prefer unwrap_or_else to unwrap_or in case of function calls/allocations#55014
Prefer unwrap_or_else to unwrap_or in case of function calls/allocations#55014bors merged 1 commit intorust-lang:masterfrom
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
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 |
7 participants
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).