Code review: Reverse temperature

5 files changed, 65 insertions, 57 deletions

diff --git a/src/layout/incremental.rs b/src/layout/incremental.rs
index b6a059c5..9eda0402 100644
--- a/src/layout/incremental.rs
+++ b/src/layout/incremental.rs
@@ -26,6 +26,11 @@ impl LayoutCache {
         self.frames.clear();
     }
 
+    /// Amount of items in the cache.
+    pub fn len(&self) -> usize {
+        self.frames.iter().map(|(_, e)| e.len()).sum()
+    }
+
     /// Retains all elements for which the closure on the level returns `true`.
     pub fn retain<F>(&mut self, mut f: F)
     where
@@ -41,9 +46,10 @@ impl LayoutCache {
         self.age += 1;
         for entry in self.frames.iter_mut().flat_map(|(_, x)| x.iter_mut()) {
             for i in 0 .. (entry.temperature.len() - 1) {
-                entry.temperature[i] = entry.temperature[i + 1];
+                entry.temperature[i + 1] = entry.temperature[i];
             }
-            *entry.temperature.last_mut().unwrap() = 0;
+            entry.temperature[0] = 0;
+            entry.age += 1;
         }
     }
 
@@ -99,12 +105,11 @@ pub struct FramesEntry {
     pub frames: Vec<Constrained<Rc<Frame>>>,
     /// How nested the frame was in the context is was originally appearing in.
     pub level: usize,
-    /// How much the element was accessed during the last five compilations, the
-    /// most recent one being the last element. `None` variants indicate that
-    /// the element is younger than five compilations.
-    temperature: [usize; 5],
     /// For how long the element already exists.
     age: usize,
+    /// How much the element was accessed during the last five compilations, the
+    /// most recent one being the first element.
+    temperature: [usize; 5],
 }
 
 impl FramesEntry {
@@ -113,8 +118,8 @@ impl FramesEntry {
         Self {
             frames,
             level,
-            temperature: [0; 5],
             age: 1,
+            temperature: [0; 5],
         }
     }
 
@@ -126,7 +131,7 @@ impl FramesEntry {
             }
         }
 
-        self.temperature[4] = self.temperature[4] + 1;
+        self.temperature[0] += 1;
 
         Some(self.frames.clone())
     }
@@ -141,16 +146,11 @@ impl FramesEntry {
     /// been used.
     pub fn cooldown(&self) -> usize {
         let mut cycle = 0;
-        for (i, &temp) in self.temperature.iter().enumerate().rev() {
-            if self.age > i {
-                if temp > 0 {
-                    return self.temperature.len() - 1 - i;
-                }
-            } else {
+        for &temp in &self.temperature[.. self.age] {
+            if temp > 0 {
                 return cycle;
             }
-
-            cycle += 1
+            cycle += 1;
         }
 
         cycle
@@ -158,7 +158,7 @@ impl FramesEntry {
 
     /// Whether this element was used in the last compilation cycle.
     pub fn hit(&self) -> bool {
-        self.temperature.last().unwrap() != &0
+        self.temperature[0] != 0
     }
 }
 
@@ -213,10 +213,10 @@ impl Constraints {
         let base = regions.base.to_spec();
         let current = regions.current.to_spec();
 
-        current.eq_by(&self.min, |&x, y| y.map_or(true, |y| x.fits(y)))
+        current.eq_by(&self.min, |x, y| y.map_or(true, |y| x.fits(y)))
             && current.eq_by(&self.max, |x, y| y.map_or(true, |y| x < &y))
-            && current.eq_by(&self.exact, |&x, y| y.map_or(true, |y| x.approx_eq(y)))
-            && base.eq_by(&self.base, |&x, y| y.map_or(true, |y| x.approx_eq(y)))
+            && current.eq_by(&self.exact, |x, y| y.map_or(true, |y| x.approx_eq(y)))
+            && base.eq_by(&self.base, |x, y| y.map_or(true, |y| x.approx_eq(y)))
     }
 
     /// Changes all constraints by adding the `size` to them if they are `Some`.
@@ -235,18 +235,13 @@ impl Constraints {
             }
         }
 
-        self.exact = override_if_some(self.exact, regions.current.to_spec());
-        self.base = override_if_some(self.base, regions.base.to_spec());
-    }
-}
+        let current = regions.current.to_spec();
+        let base = regions.base.to_spec();
 
-fn override_if_some(
-    one: Spec<Option<Length>>,
-    other: Spec<Length>,
-) -> Spec<Option<Length>> {
-    Spec {
-        vertical: one.vertical.map(|_| other.vertical),
-        horizontal: one.horizontal.map(|_| other.horizontal),
+        self.exact.horizontal.set_if_some(current.horizontal);
+        self.exact.vertical.set_if_some(current.vertical);
+        self.base.horizontal.set_if_some(base.horizontal);
+        self.base.vertical.set_if_some(base.vertical);
     }
 }
 
@@ -272,9 +267,13 @@ pub trait OptionExt {
     // Sets `other` as the value if the Option is `None` or if it contains a
     // value larger than `other`.
     fn set_min(&mut self, other: Length);
+
     // Sets `other` as the value if the Option is `None` or if it contains a
     // value smaller than `other`.
     fn set_max(&mut self, other: Length);
+
+    /// Sets `other` as the value if the Option is `Some`.
+    fn set_if_some(&mut self, other: Length);
 }
 
 impl OptionExt for Option<Length> {
@@ -291,4 +290,10 @@ impl OptionExt for Option<Length> {
             None => *self = Some(other),
         }
     }
+
+    fn set_if_some(&mut self, other: Length) {
+        if self.is_some() {
+            *self = Some(other);
+        }
+    }
 }
diff --git a/src/layout/pad.rs b/src/layout/pad.rs
index f886bec8..9461f3ff 100644
--- a/src/layout/pad.rs
+++ b/src/layout/pad.rs
@@ -15,10 +15,11 @@ impl Layout for PadNode {
         ctx: &mut LayoutContext,
         regions: &Regions,
     ) -> Vec<Constrained<Rc<Frame>>> {
-        let mut original = regions.clone();
-        let mut regions = regions.map(|size| size - self.padding.resolve(size).size());
-
-        let mut frames = self.child.layout(ctx, &regions);
+        let mut regions = regions.clone();
+        let mut frames = self.child.layout(
+            ctx,
+            &regions.map(|size| size - self.padding.resolve(size).size()),
+        );
 
         for frame in &mut frames {
             let padded = solve(self.padding, frame.size);
@@ -29,19 +30,19 @@ impl Layout for PadNode {
             let prev = std::mem::take(&mut frame.item);
             new.push_frame(origin, prev);
 
-            frame.constraints.mutate(padding.size(), &original);
+            frame.constraints.mutate(padding.size(), &regions);
 
             if self.padding.left.is_relative() || self.padding.right.is_relative() {
-                frame.constraints.base.horizontal = Some(original.base.width);
+                frame.constraints.base.horizontal = Some(regions.base.width);
             }
             if self.padding.top.is_relative() || self.padding.bottom.is_relative() {
-                frame.constraints.base.vertical = Some(original.base.height);
+                frame.constraints.base.vertical = Some(regions.base.height);
             }
 
             regions.next();
-            original.next();
             *Rc::make_mut(&mut frame.item) = new;
         }
+
         frames
     }
 }
diff --git a/src/layout/par.rs b/src/layout/par.rs
index 504981e6..45eefe29 100644
--- a/src/layout/par.rs
+++ b/src/layout/par.rs
@@ -217,9 +217,11 @@ impl<'a> ParLayouter<'a> {
                 // Again, the line must not fit. It would if the space taken up
                 // plus the line height would fit, therefore the constraint
                 // below.
-                stack.constraints.max.vertical.set_min(
-                    stack.full.height - stack.regions.current.height + line.size.height,
-                );
+                stack
+                    .constraints
+                    .max
+                    .vertical
+                    .set_min(stack.size.height + line.size.height);
                 stack.finish_region(ctx);
             }
 
@@ -230,9 +232,11 @@ impl<'a> ParLayouter<'a> {
                     break;
                 }
 
-                stack.constraints.max.vertical.set_min(
-                    stack.full.height - stack.regions.current.height + line.size.height,
-                );
+                stack
+                    .constraints
+                    .max
+                    .vertical
+                    .set_min(stack.size.height + line.size.height);
                 stack.finish_region(ctx);
             }
             // If the line does not fit horizontally or we have a mandatory
diff --git a/src/layout/stack.rs b/src/layout/stack.rs
index 045a06e1..fa5fd584 100644
--- a/src/layout/stack.rs
+++ b/src/layout/stack.rs
@@ -88,12 +88,12 @@ impl<'a> StackLayouter<'a> {
         Self {
             stack,
             main,
-            constraints: Constraints::new(expand),
             expand,
             regions,
             full,
             used: Gen::zero(),
             ruler: Align::Start,
+            constraints: Constraints::new(expand),
             overflowing: false,
             frames: vec![],
             finished: vec![],
@@ -154,7 +154,7 @@ impl<'a> StackLayouter<'a> {
             self.constraints
                 .max
                 .get_mut(self.main)
-                .set_min(size.main + self.used.main);
+                .set_min(self.used.main + size.main);
             self.finish_region();
         }
 
diff --git a/tests/typeset.rs b/tests/typeset.rs
index 740d9a03..31da3ce0 100644
--- a/tests/typeset.rs
+++ b/tests/typeset.rs
@@ -15,7 +15,7 @@ use walkdir::WalkDir;
 
 use typst::cache::Cache;
 use typst::color;
-use typst::diag::{Diag, DiagSet, Level, Pass};
+use typst::diag::{Diag, DiagSet, Level};
 use typst::eval::{eval, EvalContext, FuncArgs, FuncValue, Scope, Value};
 use typst::exec::{exec, State};
 use typst::geom::{self, Length, Point, Sides, Size};
@@ -233,14 +233,12 @@ fn test_part(
         &scope,
     );
     let executed = exec(&evaluated.output.template, state.clone());
-    let layouted = layout(loader, cache, &executed.output);
+    let mut layouted = layout(loader, cache, &executed.output);
 
     let mut diags = parsed.diags;
     diags.extend(evaluated.diags);
     diags.extend(executed.diags);
 
-    let mut pass = Pass::new(layouted, diags);
-
     let mut ok = true;
 
     for panic in &*panics.borrow() {
@@ -255,11 +253,11 @@ fn test_part(
         ok = false;
     }
 
-    if pass.diags != ref_diags {
+    if diags != ref_diags {
         println!("  Subtest {} does not match expected diagnostics. ❌", i);
         ok = false;
 
-        for diag in &pass.diags {
+        for diag in &diags {
             if !ref_diags.contains(diag) {
                 print!("    Not annotated | ");
                 print_diag(diag, &map, lines);
@@ -267,7 +265,7 @@ fn test_part(
         }
 
         for diag in &ref_diags {
-            if !pass.diags.contains(diag) {
+            if !diags.contains(diag) {
                 print!("    Not emitted   | ");
                 print_diag(diag, &map, lines);
             }
@@ -302,7 +300,7 @@ fn test_part(
             );
         }
 
-        if cached_result != pass.output {
+        if cached_result != layouted {
             ok = false;
             println!(
                 "    Recompilation of subtest {} differs from clean pass ❌",
@@ -315,10 +313,10 @@ fn test_part(
     cache.layout.turnaround();
 
     if !compare_ref {
-        pass.output.clear();
+        layouted.clear();
     }
 
-    (ok, compare_ref, pass.output)
+    (ok, compare_ref, layouted)
 }
 
 fn parse_metadata(src: &str, map: &LineMap) -> (Option<bool>, DiagSet) {