librustc: Copy or move upvars by value [breaking-change].

[pnkfelix]

librustc: Copy or move upvars by value [breaking-change].

This is a commit-refactored version of PR #14501.

(pnkfelix will rebase atop master as necessary; he just wanted to transition from pcwalton's PR over to this one cleanly first.)

Original PR message follows.

Closes #12831.

This breaks many closures (about 10%) that look like this:

let mut sum = 0;
f.map(|x| sum += x);

Instead, rewrite them like this:

let mut sum = 0;
{
let sum_ptr = &mut sum;
f.map(|x| *sum_ptr += x);
}

Or, ideally, switch to RAII- or iterator-like patterns.

[breaking-change]

[pnkfelix]

So, just for the record, this history edited version (as of commit b846820) ends in a code base that is nearly identical to pcwalton's #14501. It basically just has some bug fixes to let me get make check-stage{1,2} running locally on my mac.

Here is the complete diff between the two, as of commit b846820. (I keep adding that qualification because I will need to rebase it atop master before merging, and that will make the diff comparison between this and PR #14501 useless.)

The diff:

% git diff pcw-by-value-upvars
diff --git a/src/doc/guide-container.md b/src/doc/guide-container.md
index 30bfd28..9acb38a 100644
--- a/src/doc/guide-container.md
+++ b/src/doc/guide-container.md
@@ -184,8 +184,9 @@ let xs = [1,2,3,4,5];
 let mut calls = 0;

 {
+    let calls_ref = &mut calls;
     let it = xs.iter().scan((), |_, x| {
-        calls += 1;
+        *calls_ref += 1;
         if *x < 3 { Some(x) } else { None }});

     // the iterator will only yield 1 and 2 before returning None
diff --git a/src/doc/tutorial.md b/src/doc/tutorial.md
index 011d1b2..79b05d3 100644
--- a/src/doc/tutorial.md
+++ b/src/doc/tutorial.md
@@ -1838,7 +1838,8 @@ access local variables in the enclosing scope.

 let mut max = 0;
-let f = |x: int| if x > max { max = x };
+let max_ref = &mut max;
+let f = |x: int| if x > *max_ref { *max_ref = x };
 for x in [1, 2, 3].iter() {
     f(*x);
 }
diff --git a/src/librustc/util/common.rs b/src/librustc/util/common.rs
index 66da8f1..02de185 100644
--- a/src/librustc/util/common.rs
+++ b/src/librustc/util/common.rs
@@ -31,7 +31,7 @@ impl Timer {
         depth.replace(Some(old_depth + 1));

         Timer {
-            start: time::precise_time_s(),
+            start: if enabled { time::precise_time_s() } else { 0f64 },
             what: what,
             enabled: enabled,
             old_depth: old_depth,
@@ -41,12 +41,14 @@ impl Timer {

 impl Drop for Timer {
     fn drop(&mut self) {
-        let end = time::precise_time_s();
         depth.replace(Some(self.old_depth));
-        println!("{}time: {:3.3f} s\t{}",
-                 "  ".repeat(self.old_depth),
-                 end - self.start,
-                 self.what);
+        if self.enabled {
+            let end = time::precise_time_s();
+            println!("{}time: {:3.3f} s\t{}",
+                     "  ".repeat(self.old_depth),
+                     end - self.start,
+                     self.what);
+        }
     }
 }

diff --git a/src/llvm b/src/llvm
index 4b4d053..0a89464 160000
--- a/src/llvm
+++ b/src/llvm
@@ -1 +1 @@
-Subproject commit 4b4d0533b4f76cc3fbba31bd9e7ac02e0c738b1d
+Subproject commit 0a894645cf120539876e9eb4eb0d7b572dfa9d14

[alexcrichton]

It's probably too late now, but I would much rather shadow outer variables rather than suffix them all with _ptr.

[alexcrichton]

Just wanted to make sure there was a test for something like this as well:

let a = box 1;
let b = || { println!("{}", a) };
drop(b);
// no memory should leak

[pnkfelix]

@alexcrichton it is not too late, its easy to rebase -i and edit commits, especially since I will need to rebase it on top of master before attempting to land this anyway. (I've put this PR on the mtg agenda for tonight, just to double-check everyone is okay with the shift in the character of the language.)

[huonw]

I noticed that many of the captures are written like

let mut foo: T = init();
let foo_ptr: &mut T = &mut foo_ptr;
...

but can be written like

let mut foo: &mut T = &mut init();
...

For closures where foo_ptr did not need to be explicitly derefed (i.e. auto-deref handled everything), this should be the only change required for this patch.

This is especially true of many of the closures in tests and benchmarks.

[pnkfelix]

@huonw I don't suppose you have any ready estimate as to how many of the changes your suggestion would apply to? (My plan, based on the discussion at last night's meeting, was to just close this PR, but your comment has made me wonder if I should try to clean it up further, and refloat the idea of just doing by-value-upvars, or at least make that the easy default.)

[pnkfelix]

closing; we are reconsidering forcing all closures to move to by-value upvars : see rust-lang/rfcs#114