In Option::get_or_insert_with(), forget the None instead of dropping it.#148562
In Option::get_or_insert_with(), forget the None instead of dropping it.#148562rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Option::get_or_insert_with(), forget the None instead of dropping it.#148562Conversation
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
|
cc @cyb0124 (this change is based on their idea) |
ecc05f4 to
2e71187
Compare
2e71187 to
cbc408f
Compare
…ing it. This allows eliminating the `T: [const] Destruct` bounds and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything. Ideally, the compiler would prove that that drop is not necessary itself, but it currently doesn't, even with `const_precise_live_drops` enabled.
cbc408f to
c1b51dd
Compare
|
The implementation looks good to me. r? libs-api for the unstable API change. |
|
Error: Unknown labels: S-waiting-on-libs-api Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
Can you check the final optimized runtime MIR before and after your change? I'm not sure we're optimizing the new code properly. Also wondering about this pattern in general. Seems like sth const precise live drops could also handle, haven't looked at that in forever so I don't remember the details of it. Anyway, if the final runtime MIR looks good, let's merge this |
I’m not familiar with how to do that, but I will look into it.
See prior discussion #148562 (comment) — in brief, having |
|
oli-obk and I have looked at the MIR through dropping both implementations into Compiler Explorer and it seems generally similar, but also not very optimized either before or after. @rustbot label +const-hack |
|
Error: Unknown labels: const-hack Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
📋 This PR cannot be approved because it currently has the following label: |
This comment has been minimized.
This comment has been minimized.
In `Option::get_or_insert_with()`, forget the `None` instead of dropping it. Per #148486 (comment) In `Option::get_or_insert_with()`, after replacing the `None` with `Some`, forget the `None` instead of dropping it. This allows eliminating the `T: [const] Destruct` bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything (and which might even persist after optimization).
In `Option::get_or_insert_with()`, forget the `None` instead of dropping it. Per rust-lang#148486 (comment) In `Option::get_or_insert_with()`, after replacing the `None` with `Some`, forget the `None` instead of dropping it. This allows eliminating the `T: [const] Destruct` bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything (and which might even persist after optimization).
|
@bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #153628. |
Rollup of 8 pull requests Successful merges: - #148562 (In `Option::get_or_insert_with()`, forget the `None` instead of dropping it.) - #149931 (rustdoc: don't give depreciation notes special handling) - #153608 (ast_passes: unsupported arch w/ scalable vectors) - #153609 (Add missing `Diag::with_span_suggestion_with_style` method) - #153610 (Only lint unused features if they are unstable) - #153616 (Update `sysinfo` version to `0.38.4`) - #153619 (Update books) - #153624 (Ping fmease on parser modifications)
This comment has been minimized.
This comment has been minimized.
In `Option::get_or_insert_with()`, forget the `None` instead of dropping it. Per #148486 (comment) In `Option::get_or_insert_with()`, after replacing the `None` with `Some`, forget the `None` instead of dropping it. This allows eliminating the `T: [const] Destruct` bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything (and which might even persist after optimization).
|
The @bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #153631. |
Rollup of 4 pull requests Successful merges: - #148562 (In `Option::get_or_insert_with()`, forget the `None` instead of dropping it.) - #153325 (tests/ui/cfg: add annotations for reference rules) - #153621 (Remove `TyCtxt::node_span_lint` method) - #153627 (rustdoc-json: Improve docs for `ItemEnum::item_kind`)
This comment has been minimized.
This comment has been minimized.
In `Option::get_or_insert_with()`, forget the `None` instead of dropping it. Per #148486 (comment) In `Option::get_or_insert_with()`, after replacing the `None` with `Some`, forget the `None` instead of dropping it. This allows eliminating the `T: [const] Destruct` bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything (and which might even persist after optimization).
|
💔 Test for 8744ff0 failed: CI. Failed job:
|
|
@bors retry (network failure) |
Rollup of 4 pull requests Successful merges: - #148562 (In `Option::get_or_insert_with()`, forget the `None` instead of dropping it.) - #153325 (tests/ui/cfg: add annotations for reference rules) - #153621 (Remove `TyCtxt::node_span_lint` method) - #153627 (rustdoc-json: Improve docs for `ItemEnum::item_kind`)
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
Rollup of 4 pull requests Successful merges: - #148562 (In `Option::get_or_insert_with()`, forget the `None` instead of dropping it.) - #153325 (tests/ui/cfg: add annotations for reference rules) - #153621 (Remove `TyCtxt::node_span_lint` method) - #153627 (rustdoc-json: Improve docs for `ItemEnum::item_kind`)
Rollup merge of #148562 - kpreid:get-init-drop, r=oli-obk In `Option::get_or_insert_with()`, forget the `None` instead of dropping it. Per #148486 (comment) In `Option::get_or_insert_with()`, after replacing the `None` with `Some`, forget the `None` instead of dropping it. This allows eliminating the `T: [const] Destruct` bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit `drop_in_place::<Option<T>>()` that will never do anything (and which might even persist after optimization).
View all comments
Per #148486 (comment)
In
Option::get_or_insert_with(), after replacing theNonewithSome, forget theNoneinstead of dropping it.This allows eliminating the
T: [const] Destructbounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicitdrop_in_place::<Option<T>>()that will never do anything (and which might even persist after optimization).