Add Cell::update#49727

ghost · 2018-04-06T13:16:36Z

This commit adds a new method Cell::update, which applies a function to the value inside the cell.

Previously discussed in: rust-lang/rfcs#2171

Motivation

Updating Cells is currently a bit verbose. Here are several real examples (taken from rustc and crossbeam):

self.print_fuel.set(self.print_fuel.get() + 1);

self.diverges.set(self.diverges.get() | Diverges::Always);

let guard_count = self.guard_count.get();
self.guard_count.set(guard_count.checked_add(1).unwrap());
if guard_count == 0 {
    // ...
}

With the addition of the new method Cell::update, this code can be simplified to:

self.print_fuel.update(|x| x + 1);

self.diverges.update(|x| x | Diverges::Always);

if self.guard_count.update(|x| x.checked_add(1).unwrap()) == 1 {
    // ...
}

Unresolved questions

Should we return the old value instead of the new value (like in fetch_add and fetch_update)?
Should the return type simply be ()?
Naming: update vs modify vs mutate etc.

cc @SimonSapin

rust-highfive · 2018-04-06T13:16:40Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

TimNN · 2018-04-06T13:23:37Z

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:00:59] configure: rust.quiet-tests     := True
---
[00:05:24] tidy error: /checkout/src/libcore/cell.rs:273: TODO is deprecated; use FIXME
[00:05:26] some tidy checks failed
[00:05:26]
[00:05:26]
[00:05:26] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:26] expected success, got: exit code: 1
[00:05:26]
[00:05:26]
[00:05:26] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:26] Build completed unsuccessfully in 0:02:19
[00:05:26] make: *** [tidy] Error 1
[00:05:26] Makefile:79: recipe for target 'tidy' failed
---
10012 ./src/llvm/test/MC/AMDGPU
9648 ./src/llvm/test/MC/Disassembler/AMDGPU
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:081cefc6:start=1523021014516179890,finish=1523021014522192257,duration=6012367
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:001d9000
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:001d9000:start=1523021014527841610,finish=1523021014533952170,duration=6110560
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0e1374b8
$ dmesg | grep -i kill
[   10.456863] init: failsafe main process (1093) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

ghost · 2018-04-06T13:24:42Z

Here are a few more real-word examples of the x.set(something with x.get()) pattern.

In this repository (click to expand):

src/libarena/lib.rs
147:                self.ptr.set(self.ptr.get().offset(1));
543:            self.count.set(self.count.get() + 1);

src/test/run-pass/resource-assign-is-not-copy.rs
20:        self.i.set(self.i.get() + 1);

src/test/run-pass/vec-slice-drop.rs
20:        self.x.set(self.x.get() + 1);

src/test/run-pass/traits-conditional-model-fn.rs
33:        self.counter.set(self.counter.get() + arg);
45:        self.counter.set(self.counter.get() + arg);

src/test/run-pass/overloaded-autoderef-count.rs
39:        self.count_imm.set(self.count_imm.get() + 1);

src/test/run-pass/issue-16492.rs
32:        self.state.set(self.state.get()+1);

src/test/run-pass/option-unwrap.rs
19:        self.x.set(self.x.get() - 1);

src/test/run-pass/dynamic-drop.rs
53:        self.cur_ops.set(self.cur_ops.get() + 1);
76:        self.1.cur_ops.set(self.1.cur_ops.get()+1);

src/test/run-pass/packed-struct-drop-aligned.rs
27:        self.drop_count.set(self.drop_count.get() + 1);

src/test/run-pass/overloaded-deref-count.rs
40:        self.count_imm.set(self.count_imm.get() + 1);

src/test/run-pass/resource-destruct.rs
19:        println!("Hello!"); self.i.set(self.i.get() - 1);

src/test/run-pass/issue-979.rs
19:        self.b.set(self.b.get() + 1);

src/test/run-pass/init-res-into-things.rs
26:        self.i.set(self.i.get() + 1)

src/test/pretty/block-disambig.rs
60:    match true { true => { } _ => { } } regs.set(regs.get() + 1);

src/liballoc/tests/slice.rs
1407:        self.version.set(self.version.get() + 1);
1408:        other.version.set(other.version.get() + 1);

src/librustc_typeck/check/_match.rs
610:            self.diverges.set(self.diverges.get() | Diverges::Always);

src/librustc_typeck/check/mod.rs
3601:            self.diverges.set(self.diverges.get() | Diverges::Always);
3610:        self.diverges.set(self.diverges.get() | old_diverges);
3611:        self.has_errors.set(self.has_errors.get() | old_has_errors);
4362:        self.diverges.set(self.diverges.get() | old_diverges);
4363:        self.has_errors.set(self.has_errors.get() | old_has_errors);

src/librustc/util/common.rs
237:    accu.set(duration + accu.get());
384:        self.set(self.get() + 1);

src/librustc/session/mod.rs
899:                self.print_fuel.set(self.print_fuel.get() + 1);

In Servo (click to expand):

work $ cd servo/
components/layout_thread/lib.rs
1632:        self.generation.set(self.generation.get() + 1);

components/script/timers.rs
250:        self.suspension_offset.set(self.suspension_offset.get() + additional_offset);

components/script/dom/webglshader.rs
190:        self.attached_counter.set(self.attached_counter.get() + 1);
195:        self.attached_counter.set(self.attached_counter.get() - 1);

components/script/dom/window.rs
1580:        self.pending_reflow_count.set(self.pending_reflow_count.get() + 1);

components/script/dom/node.rs
257:        self.children_count.set(self.children_count.get() + 1);
297:        self.children_count.set(self.children_count.get() - 1);

components/script/dom/htmllinkelement.rs
295:        self.request_generation_id.set(self.request_generation_id.get().increment());
319:        self.pending_loads.set(self.pending_loads.get() + 1)
328:        self.pending_loads.set(self.pending_loads.get() - 1);

components/script/dom/htmlmediaelement.rs
680:        self.generation_id.set(self.generation_id.get() + 1);

components/script/dom/htmlstyleelement.rs
202:        self.pending_loads.set(self.pending_loads.get() + 1)
211:        self.pending_loads.set(self.pending_loads.get() - 1);

components/script/dom/htmlimageelement.rs
603:        self.generation.set(self.generation.get() + 1);

components/script/dom/document.rs
551:        self.dom_count.set(self.dom_count.get() + 1);
556:        self.dom_count.set(self.dom_count.get() - 1);
1384:        count_cell.set(count_cell.get() + 1);
1390:        count_cell.set(count_cell.get() - 1);
1507:                self.spurious_animation_frames.set(self.spurious_animation_frames.get() + 1)

components/gfx/tests/font_context.rs
87:        self.find_font_count.set(self.find_font_count.get() + 1);

TimNN · 2018-04-06T14:47:23Z

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:01:05] configure: rust.quiet-tests     := True
---
[00:44:32] ..........................................................................i.........................
[00:44:38] .................i..................................................................................
---
[00:45:16] ..............................................................................................i.....
[00:45:24] .....................................................................i..............................
---
[00:46:24] .............................................i......................................................
---
[00:50:30] .............................i......................................................................
[00:50:45] ..............................................................i.....................................
[00:51:01] ...............................................i....................................................
[00:51:22] ....................................................................................................
[00:51:45] ....................................................................................................
[00:52:07] ....................................................................................................
[00:52:33] ..i...............................................................................................i.
[00:52:59] ............................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:53:10] ........................
[00:53:42] ....................................................................................................
[00:54:20] ..............................................................ii....................................
[00:55:04] .........................i..................................................test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:55:14] ..i.ii..................
[00:55:58] ......................................................................................iiiiiii.......
---
[00:58:14] ..................i............................................................ii.iii...............
[00:58:21] ....................................................................................................
[00:58:29] ........i..............................i............................................................
[00:58:37] ....................................................................................................
[00:58:44] ....................i...............................................................................
[00:58:52] ....................................................................................................
[00:59:02] ....................................................................................................
[00:59:13] ....................................................................................................
[00:59:24] ....................................................................................................
[00:59:38] ....................................................................................................
[00:59:47] .............i......................................................................................
[00:59:57] .................i..ii..............................................................................
[01:00:07] ....................................................................................................
[01:00:17] ....................................................................................................
[01:00:27] ....................................................................................i...............
[01:00:37] ..............................i.....................................................................
---
[01:01:14] ...........................i........................................................................
[01:01:16] ....................................................................i...............................
[01:01:17] ................i.......................................................
---
[01:01:32] ...........i........................
---
[01:02:07] i...i..ii....i.............ii........iii......i..i...i...ii..i..i..ii.....
---
[01:02:10] i.......i......................i......
---
[01:02:49] iiii.......i..i........i..i.i.............i..........iiii...........i...i..........ii.i.i.......ii..
[01:02:50] ....ii...
---
[01:12:39] .....i..............................................................................................
---
[01:14:49] .....................................i..............................................................
[01:15:13] ....................................................................................................
[01:15:36] ..........................................i.........................................................
---
[01:16:15] error[E0658]: use of unstable library feature 'cell_update' (see issue #0)
[01:16:15]   --> libcore/../libcore/tests/cell.rs:33:18
[01:16:15]    |
[01:16:15] 33 |     assert_eq!(x.update(|x| x + 5), 15);
[01:16:15]    |                  ^^^^^^
[01:16:15]    |
[01:16:15]    = help: add #![feature(cell_update)] to the crate attributes to enable
[01:16:15]
[01:16:15] error[E0658]: use of unstable library feature 'cell_update' (see issue #0)
[01:16:15]   --> libcore/../libcore/tests/cell.rs:36:18
[01:16:15]    |
[01:16:15] 36 |     assert_eq!(x.update(|x| x / 3), 5);
[01:16:15]    |                  ^^^^^^
[01:16:15]    |
[01:16:15]    = help: add #![feature(cell_update)] to the crate attributes to enable
---
[01:16:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:16:28] expected success, got: exit code: 101
[01:16:28]
[01:16:28]
[01:16:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:16:28] Build completed unsuccessfully in 0:33:12
[01:16:28] Makefile:58: recipe for target 'check' failed
[01:16:28] make: *** [check] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:280779f6:start=1523026034223521353,finish=1523026034240608135,duration=17086782
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:1463d669
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:1463d669:start=1523026034247167254,finish=1523026034255352293,duration=8185039
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02a13b2a
$ dmesg | grep -i kill
[   10.383097] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

emilyalbini · 2018-04-16T10:22:33Z

Thanks for this PR! The triage team will make sure @cramertj (or someone else from @rust-lang/libs) reviews this.

cramertj · 2018-04-16T16:29:13Z

r? @SimonSapin (someone from libs)

SimonSapin · 2018-04-16T17:50:26Z

Looks good to me. @stjepang, could you open a tracking issue and point the #[unstable] attribute to it? Also it’s not super obvious that the returned value should be the new one (v.s. the old one, or nothing at all). Could mention it in the documentation prose and show it in the doctest?

emilyalbini · 2018-04-23T12:01:30Z

Ping from triage @stjepang! There is some action needed on your part.

ghost · 2018-04-23T18:36:42Z

Done - I've clarified the docs and opened a tracking issue: #50186.

SimonSapin · 2018-04-23T18:38:27Z

Perfect, thanks!

@bors: r+ rollup

bors · 2018-04-23T18:38:28Z

📌 Commit 29e9de8 has been approved by SimonSapin

@SimonSapin

Add Cell::update This commit adds a new method `Cell::update`, which applies a function to the value inside the cell. Previously discussed in: rust-lang/rfcs#2171 ### Motivation Updating `Cell`s is currently a bit verbose. Here are several real examples (taken from rustc and crossbeam): ```rust self.print_fuel.set(self.print_fuel.get() + 1); self.diverges.set(self.diverges.get() | Diverges::Always); let guard_count = self.guard_count.get(); self.guard_count.set(guard_count.checked_add(1).unwrap()); if guard_count == 0 { // ... } ``` With the addition of the new method `Cell::update`, this code can be simplified to: ```rust self.print_fuel.update(|x| x + 1); self.diverges.update(|x| x | Diverges::Always); if self.guard_count.update(|x| x.checked_add(1).unwrap()) == 1 { // ... } ``` ### Unresolved questions 1. Should we return the old value instead of the new value (like in `fetch_add` and `fetch_update`)? 2. Should the return type simply be `()`? 3. Naming: `update` vs `modify` vs `mutate` etc. cc @SimonSapin

Rollup of 11 pull requests Successful merges: - #49461 (std: Child::kill() returns error if process has already exited) - #49727 (Add Cell::update) - #49812 (Fix revision support for UI tests.) - #49829 (Add doc links to `std::os` extension traits) - #49906 (Stabilize `std::hint::unreachable_unchecked`.) - #49970 (Deprecate Read::chars and char::decode_utf8) - #49985 (don't see issue #0) - #50118 (fix search bar bug) - #50139 (encourage descriptive issue titles) - #50174 (Use FxHashMap in syntax_pos::symbol::Interner::intern.) - #50185 (core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`) Failed merges:

peterjoel · 2019-05-13T13:24:25Z

Related conversation on a similar PR (for RefCell): #38741

(Wasn't sure if this kind of comment is more suitable for the PR or the tracking issue)

SimonSapin · 2019-05-13T15:08:59Z

In general, merged or closed PRs are considered done and are not a great place to have further conversations. This is why we have tracking issues :)

peterjoel · 2019-05-13T15:36:43Z

@SimonSapin Ah, I didn't notice that this was merged!

Add Cell::update

f86deef

rust-highfive assigned cramertj Apr 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2018

Change TODO to FIXME

9377340

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 6, 2018

Fix the failing tests

5dcce51

rust-highfive assigned SimonSapin and unassigned cramertj Apr 16, 2018

emilyalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2018

Stjepan Glavina added 2 commits April 23, 2018 20:23

Clarify the docs for Cell::update

1c0db24

Assign the tracking issue

29e9de8

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2018

kennytm mentioned this pull request Apr 24, 2018

Rollup of 11 pull requests #50191

Merged

bors merged commit 29e9de8 into rust-lang:master Apr 24, 2018

ghost deleted the cell-update branch April 24, 2018 09:26

frol mentioned this pull request Jun 20, 2018

RFC: Add Option::replace to the core library rust-lang/rfcs#2296

Merged

ghost mentioned this pull request Oct 16, 2018

Unchecked thread spawning #55043

Merged

Uh oh!

Conversation

ghost commented Apr 6, 2018

Motivation

Unresolved questions

Uh oh!

rust-highfive commented Apr 6, 2018

Uh oh!

TimNN commented Apr 6, 2018

Uh oh!

ghost commented Apr 6, 2018

Uh oh!

TimNN commented Apr 6, 2018

Uh oh!

emilyalbini commented Apr 16, 2018

Uh oh!

cramertj commented Apr 16, 2018

Uh oh!

SimonSapin commented Apr 16, 2018

Uh oh!

emilyalbini commented Apr 23, 2018

Uh oh!

ghost commented Apr 23, 2018

Uh oh!

SimonSapin commented Apr 23, 2018

Uh oh!

bors commented Apr 23, 2018

Uh oh!

peterjoel commented May 13, 2019

Uh oh!

SimonSapin commented May 13, 2019

Uh oh!

peterjoel commented May 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants