ronantakizawa/codereview-bench huggingface dataset:
Repository: typst/typst
File: crates/typst-library/src/text/quote.rs (Rust)

Code before:
use super::{SmartquoteElem, SpaceElem, TextElem};
use crate::layout::{BlockElem, HElem, PadElem, Spacing, VElem};
use crate::meta::{CitationForm, CiteElem};
use crate::prelude::*;

/// Displays a quote alongside an attribution.
///
/// # Example
/// ```example
/// Plato is often misquoted as the author of #quote[I know that I know
/// nothing], however, this is a derivation form his original quote:
/// #set quote(block: true)
/// #quote(attribution: [Plato])[
///   ... ἔοικα γοῦν τούτου γε σμικρῷ τινι αὐτῷ τούτῳ σοφώτερος εἶναι, ὅτι
///   ἃ μὴ οἶδα οὐδὲ οἴομαι εἰδέναι.
/// ]
/// #quote(attribution: [from the Henry Cary literal translation of 1897])[
///   ... I seem, then, in just this little thing to be wiser than this man at
///   any rate, that what I do not know I do not think I know either.
/// ]
/// ```
///
/// By default block quotes are padded left and right by `{1em}`, alignment and
/// padding can be controlled with show rules:
/// ```example
/// #set quote(block: true)
/// #show quote: set align(center)
/// #show quote: set pad(x: 5em)
///
/// #quote[
///   You cannot pass... I am a servant of the Secret Fire, wielder of the

Review comment (suggestion):
```suggestion
/// Displays a quote alongside an optional attribution.
```

Code after:
use super::{SmartquoteElem, SpaceElem, TextElem};
use crate::layout::{BlockElem, HElem, PadElem, Spacing, VElem};
use crate::meta::{CitationForm, CiteElem};
use crate::prelude::*;

/// Displays a quote alongside an optional attribution.
///
/// # Example
/// ```example
/// Plato is often misquoted as the author of #quote[I know that I know
/// nothing], however, this is a derivation form his original quote:
///
/// #set quote(block: true)
///
/// #quote(attribution: [Plato])[
///   ... ἔοικα γοῦν τούτου γε σμικρῷ τινι αὐτῷ τούτῳ σοφώτερος εἶναι, ὅτι
///   ἃ μὴ οἶδα οὐδὲ οἴομαι εἰδέναι.
/// ]
/// #quote(attribution: [from the Henry Cary literal translation of 1897])[
///   ... I seem, then, in just this little thing to be wiser than this man at
///   any rate, that what I do not know I do not think I know either.
/// ]
/// ```
///
/// By default block quotes are padded left and right by `{1em}`, alignment and
/// padding can be controlled with show rules:
/// ```example
/// #set quote(block: true)
/// #show quote: set align(center)
/// #show quote: set pad(x: 5em)
///
Repository: typst/typst
File: tests/src/tests.rs (Rust)

Code before:
    #[arg(long)]
    frames: bool,
}

impl Args {
    fn matches(&self, path: &Path) -> bool {
        if !self.exact {
            let path = path.to_string_lossy();
            return self.filter.is_empty()
                || self.filter.iter().any(|v| path.contains(v));
        }

        let components: Vec<String> = path
            .components()
            .map(|c| c.as_os_str().to_string_lossy().into_owned())
            .collect();
        let components_len = components.len();
        let mut path_endings: HashSet<String> = HashSet::new();
        for i in 0..components_len {
            path_endings.insert(
                components[(components_len - i - 1)..]
                    .join(std::path::MAIN_SEPARATOR_STR),
            );
        }

        self.filter.iter().any(|v| path_endings.contains(v))
    }
}

fn main() {
    let args = Args::parse();

    // Create loader and context.
    let world = TestWorld::new(args.print);

    println!("Running tests...");
    let results = WalkDir::new(TYP_DIR)
        .into_iter()
        .par_bridge()
        .filter_map(|entry| {
            let entry = entry.unwrap();
            if entry.depth() == 0 {
                return None;
            }

            if entry.path().starts_with("typ/benches") {
                return None;
            }

            let src_path = entry.into_path(); // Relative to TYP_DIR.
            if src_path.extension() != Some(OsStr::new("typ")) {

Review comment (suggestion):
I think this could be simplified by checking whether the path ends with `v` + whether the prefix of the path before `v` is empty or ends with `/`.

Code after:
    #[arg(long)]
    frames: bool,
}

impl Args {
    fn matches(&self, canonicalized_path: &Path) -> bool {
        let path = canonicalized_path.to_string_lossy();
        if !self.exact {
            return self.filter.is_empty()
                || self.filter.iter().any(|v| path.contains(v));
        }

        self.filter.iter().any(|v| match path.strip_suffix(v) {
            None => false,
            Some(residual) => {
                residual.is_empty() || residual.ends_with(MAIN_SEPARATOR_STR)
            }
        })
    }
}

fn main() {
    let args = Args::parse();

    // Create loader and context.
    let world = TestWorld::new(args.print);

    println!("Running tests...");
    let results = WalkDir::new(TYP_DIR)
        .into_iter()
        .par_bridge()
        .filter_map(|entry| {
            let entry = entry.unwrap();
            if entry.depth() == 0 {
                return None;
            }

            if entry.path().starts_with("typ/benches") {
                return None;
            }

            let src_path = entry.into_path(); // Relative to TYP_DIR.
            if src_path.extension() != Some(OsStr::new("typ")) {
                return None;
            }

            if args.matches(&src_path.canonicalize().unwrap()) {
                Some(src_path)
            } else {
                None
            }
Repository: typst/typst
File: tests/src/benches.rs (Rust)

Code before:
    let world = BenchWorld::new();
    let mut tracer = Tracer::new();
    iai.run(|| typst::compile(&world, &mut tracer));
}

fn bench_render(iai: &mut Iai) {
    let world = BenchWorld::new();
    let mut tracer = Tracer::new();
    let document = typst::compile(&world, &mut tracer).unwrap();
    iai.run(|| typst_render::render(&document.pages[0], 1.0, Color::WHITE))
}

struct BenchWorld {
    library: Prehashed<Library>,
    book: Prehashed<FontBook>,
    font: Font,
    source: Source,
}

impl BenchWorld {
    fn new() -> Self {
        let font = Font::new(FONT.into(), 0).unwrap();
        let book = FontBook::from_fonts([&font]);

        Self {
            library: Prehashed::new(Library::build(SysArguments::default())),
            book: Prehashed::new(book),
            font,
            source: Source::detached(TEXT),
        }
    }

    fn track(&self) -> Tracked<dyn World> {
        (self as &dyn World).track()
    }
}

impl World for BenchWorld {
    fn library(&self) -> &Prehashed<Library> {
        &self.library
    }

    fn book(&self) -> &Prehashed<FontBook> {
        &self.book
    }

    fn main(&self) -> Source {
        self.source.clone()
    }

    fn source(&self, _: FileId) -> FileResult<Source> {

Review comment (suggestion):
When seeing this, I'm unsure whether `SysArguments` is really that great. Maybe it should rather be a `LibraryBuilder` struct and we have `Library::builder() -> LibraryBuilder` and `LibraryBuilder::build() -> Library`. The places for `SysArguments::default()` are used would then just use `Library::default()` and those that require further configuration can use `Library::builder().with_inputs(..).build()`.

Code after:
    let mut tracer = Tracer::new();
    iai.run(|| typst::compile(&world, &mut tracer));
}

fn bench_render(iai: &mut Iai) {
    let world = BenchWorld::new();
    let mut tracer = Tracer::new();
    let document = typst::compile(&world, &mut tracer).unwrap();
    iai.run(|| typst_render::render(&document.pages[0], 1.0, Color::WHITE))
}

struct BenchWorld {
    library: Prehashed<Library>,
    book: Prehashed<FontBook>,
    font: Font,
    source: Source,
}

impl BenchWorld {
    fn new() -> Self {
        let font = Font::new(FONT.into(), 0).unwrap();
        let book = FontBook::from_fonts([&font]);

        Self {
            library: Prehashed::new(Library::default()),
            book: Prehashed::new(book),
            font,
            source: Source::detached(TEXT),
        }
    }

    fn track(&self) -> Tracked<dyn World> {
        (self as &dyn World).track()
    }
}

impl World for BenchWorld {
    fn library(&self) -> &Prehashed<Library> {
        &self.library
    }

    fn book(&self) -> &Prehashed<FontBook> {
        &self.book
    }

    fn main(&self) -> Source {
        self.source.clone()
    }

    fn source(&self, _: FileId) -> FileResult<Source> {
        unimplemented!()
Repository: typst/typst
File: crates/typst/src/text/item.rs (Rust)

Code before:
impl TextItem {
    /// The width of the text run.
    pub fn width(&self) -> Abs {
        self.glyphs.iter().map(|g| g.x_advance).sum::<Em>().at(self.size)
    }
}

impl Debug for TextItem {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        f.write_str("Text(")?;
        self.text.fmt(f)?;
        f.write_str(")")
    }
}

/// A glyph in a run of shaped text.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Glyph {
    /// The glyph's index in the font.
    pub id: u16,
    /// The advance width of the glyph.
    pub x_advance: Em,
    /// The horizontal offset of the glyph.
    pub x_offset: Em,
    /// The range of the glyph in its item's text. The range's length may
    /// be more than one due to e.g. ligature, big Unicode codepoint value.
    pub range: Range<u16>,
    /// The source code location of the text.
    pub span: (Span, u16),
}

impl Glyph {
    /// The range of the glyph in its item's text.
    pub fn range(&self) -> Range<usize> {
        usize::from(self.range.start)..usize::from(self.range.end)
    }
}


Review comment (suggestion):
```suggestion
    /// be more than one due to multi-byte UTF-8 encoding or ligatures.
```

Code after:
impl TextItem {
    /// The width of the text run.
    pub fn width(&self) -> Abs {
        self.glyphs.iter().map(|g| g.x_advance).sum::<Em>().at(self.size)
    }
}

impl Debug for TextItem {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        f.write_str("Text(")?;
        self.text.fmt(f)?;
        f.write_str(")")
    }
}

/// A glyph in a run of shaped text.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct Glyph {
    /// The glyph's index in the font.
    pub id: u16,
    /// The advance width of the glyph.
    pub x_advance: Em,
    /// The horizontal offset of the glyph.
    pub x_offset: Em,
    /// The range of the glyph in its item's text. The range's length may
    /// be more than one due to multi-byte UTF-8 encoding or ligatures.
    pub range: Range<u16>,
    /// The source code location of the text.
    pub span: (Span, u16),
}

impl Glyph {
    /// The range of the glyph in its item's text.
    pub fn range(&self) -> Range<usize> {
        usize::from(self.range.start)..usize::from(self.range.end)
    }
}
Repository: typst/typst
File: crates/typst/src/layout/abs.rs (Rust)

Code before:
            Self::raw(result)
        } else {
            Self::zero()
        }
    }
}

impl Numeric for Abs {
    fn zero() -> Self {
        Self::zero()
    }

    fn is_finite(self) -> bool {
        self.0.is_finite()
    }
}

impl Debug for Abs {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "{:?}pt", self.to_pt())
    }
}

impl Repr for Abs {
    fn repr(&self) -> EcoString {
        repr::format_float(self.to_pt(), Some(2), false, "pt")
    }
}

impl Neg for Abs {
    type Output = Self;

    fn neg(self) -> Self {
        Self(-self.0)
    }
}

impl Add for Abs {
    type Output = Self;

    fn add(self, other: Self) -> Self {
        Self(self.0 + other.0)
    }
}

sub_impl!(Abs - Abs -> Abs);

impl Mul<f64> for Abs {
    type Output = Self;

    fn mul(self, other: f64) -> Self {

Review comment (refactor):
there are a bunch of these calls with `Some(2), false`. maybe we can also extract a semantic function for that like `display_float`. (which would then dispatch to format_float)

Code after:
            Self::raw(result)
        } else {
            Self::zero()
        }
    }
}

impl Numeric for Abs {
    fn zero() -> Self {
        Self::zero()
    }

    fn is_finite(self) -> bool {
        self.0.is_finite()
    }
}

impl Debug for Abs {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "{:?}pt", self.to_pt())
    }
}

impl Repr for Abs {
    fn repr(&self) -> EcoString {
        repr::format_float_with_unit(self.to_pt(), "pt")
    }
}

impl Neg for Abs {
    type Output = Self;

    fn neg(self) -> Self {
        Self(-self.0)
    }
}

impl Add for Abs {
    type Output = Self;

    fn add(self, other: Self) -> Self {
        Self(self.0 + other.0)
    }
}

sub_impl!(Abs - Abs -> Abs);

impl Mul<f64> for Abs {
    type Output = Self;

    fn mul(self, other: f64) -> Self {
Repository: typst/typst
File: crates/typst/src/layout/inline/mod.rs (Rust)

Code before:
                expanded.end = cursor + len;
            } else {
                break;
            }

            cursor += len;
        }

        (expanded, &self.items[start..end])
    }
}

/// A segment of one or multiple collapsed children.
#[derive(Debug, Clone)]
enum Segment<'a> {
    /// One or multiple collapsed text or text-equivalent children. Stores how
    /// long the segment is (in bytes of the full text string).
    Text(usize),
    /// Horizontal spacing between other segments.
    Spacing(Spacing),
    /// A mathematical equation.
    Equation(&'a EquationElem, Vec<MathParItem>),
    /// A box with arbitrary content and fixed width.
    Box(&'a BoxElem, Frame),
    /// A box with fractional width.
    FracBox(&'a BoxElem, Fr),
    /// Metadata.
    Meta,
}

impl Segment<'_> {
    /// The text length of the item.
    fn len(&self) -> usize {
        match *self {
            Self::Text(len) => len,
            Self::Spacing(_) => SPACING_REPLACE.len_utf8(),
            Self::FracBox(_, _) => SPACING_REPLACE.len_utf8(),
            Self::Box(_, _) => OBJ_REPLACE.len_utf8(),
            Self::Equation(_, ref par_items) => par_items
                .iter()
                .map(|c| match c {
                    MathParItem::Space(_) => SPACING_REPLACE,
                    MathParItem::Frame(_) => OBJ_REPLACE,
                })
                .map(char::len_utf8)
                .sum(),
            Self::Meta => 0,
        }
    }
}


Review comment (suggestion):
I think the fr-related changes can be reverted because the order in which these things are layouted doesn't matter, as long as it is fixed.

Code after:
                expanded.end = cursor + len;
            } else {
                break;
            }

            cursor += len;
        }

        (expanded, &self.items[start..end])
    }
}

/// A segment of one or multiple collapsed children.
#[derive(Debug, Clone)]
enum Segment<'a> {
    /// One or multiple collapsed text or text-equivalent children. Stores how
    /// long the segment is (in bytes of the full text string).
    Text(usize),
    /// Horizontal spacing between other segments.
    Spacing(Spacing),
    /// A mathematical equation.
    Equation(&'a EquationElem, Vec<MathParItem>),
    /// A box with arbitrary content.
    Box(&'a BoxElem, bool),
    /// Metadata.
    Meta,
}

impl Segment<'_> {
    /// The text length of the item.
    fn len(&self) -> usize {
        match *self {
            Self::Text(len) => len,
            Self::Spacing(_) => SPACING_REPLACE.len_utf8(),
            Self::Box(_, frac) => {
                (if frac { SPACING_REPLACE } else { OBJ_REPLACE }).len_utf8()
            }
            Self::Equation(_, ref par_items) => {
                par_items.iter().map(MathParItem::text).map(char::len_utf8).sum()
            }
            Self::Meta => 0,
        }
    }
}

/// A prepared item in a paragraph layout.
#[derive(Debug)]
enum Item<'a> {
    /// A shaped text run with consistent style and direction.
    Text(ShapedText<'a>),
    /// Absolute spacing between other items.
Repository: typst/typst
File: crates/typst/src/foundations/calc.rs (Rust)

Code before:
/// ```
pub fn bit_rshift(
    /// The operand whose bits will be shifted.
    operand: i64,

    /// The amount of bits to shift. Must not be negative.
    /// This is capped at the operand's bits minus 1 (63).
    /// Therefore, the shift will always succeed.
    shift: u32,

    /// Toggles whether a logical (unsigned) right shift should be performed
    /// instead of arithmetic right shift (the default).
    /// If this is `true`, negative operands will not preserve their sign bit,
    /// and bits which appear to the left after the shift will be 0.
    /// This parameter has no effect on non-negative operands.
    #[named]
    #[default(false)]
    logical: bool,
) -> i64 {
    // Don't panic on excessive shift.
    let shift = shift.min(i64::BITS - 1);

    if !logical || operand >= 0 {
        // For non-negative operands, arithmetic and logical shifts produce the
        // same results.
        operand >> shift
    } else {
        // For negative operands, we reinterpret their bits as unsigned to
        // perform logical right shift, and then reinterpret back as signed.
        // This is valid as, according to the Rust reference, casting between
        // two integers of same size (i64 <-> u64) is a no-op (two's complement
        // is used).
        // Reference:
        // https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#numeric-cast
        ((operand as u64) >> shift) as i64
    }
}

/// Calculates the factorial of a number.
///
/// ```example
/// #calc.fact(5)
/// ```
#[func(title = "Factorial")]
pub fn fact(
    /// The number whose factorial to calculate. Must be non-negative.
    number: u64,
) -> StrResult<i64> {
    Ok(fact_impl(1, number).ok_or("the result is too large")?)
}


Review comment (suggestion):
I think the `operand >= 0` condition is unnecessary and confusing when reading the code. I'd suggest simply performing the unsigned shift when `logical` is true, no matter what.

Code after:
    }

    a.abs()
}

/// Calculates the least common multiple of two integers.
///
/// ```example
/// #calc.lcm(96, 13)
/// ```
#[func(title = "Least Common Multiple")]
pub fn lcm(
    /// The first integer.
    a: i64,
    /// The second integer.
    b: i64,
) -> StrResult<i64> {
    if a == b {
        return Ok(a.abs());
    }

    Ok(a.checked_div(gcd(a, b))
        .and_then(|gcd| gcd.checked_mul(b))
        .map(|v| v.abs())
        .ok_or("the return value is too large")?)
}

/// Rounds a number down to the nearest integer.
///
/// If the number is already an integer, it is returned unchanged.
///
/// ```example
/// #assert(calc.floor(3.14) == 3)
/// #assert(calc.floor(3) == 3)
/// #calc.floor(500.1)
/// ```
#[func]
pub fn floor(
    /// The number to round down.
    value: Num,
) -> i64 {
    match value {
        Num::Int(n) => n,
        Num::Float(n) => n.floor() as i64,
    }
}

/// Rounds a number up to the nearest integer.
///
/// If the number is already an integer, it is returned unchanged.
///
Repository: typst/typst
File: crates/typst/src/foundations/ty.rs (Rust)

Code before:
        self.0.long_name
    }

    /// The type's title case name, for use in documentation (e.g. `String`).
    pub fn title(&self) -> &'static str {
        self.0.title
    }

    /// Documentation for the type (as Markdown).
    pub fn docs(&self) -> &'static str {
        self.0.docs
    }

    /// Search keywords for the type.
    pub fn keywords(&self) -> &'static [&'static str] {
        self.0.keywords
    }

    /// This type's constructor function.
    pub fn constructor(&self) -> StrResult<Func> {
        self.0
            .constructor
            .as_ref()
            .map(|lazy| Func::from(*lazy))
            .ok_or_else(|| {
                eco_format!("type `{}` does not have a constructor", self.short_name())
            })
    }

    /// The type's associated scope that holds sub-definitions.
    pub fn scope(&self) -> &'static Scope {
        &(self.0).0.scope
    }

    /// Get a field from this type's scope, if possible.
    pub fn field(&self, field: &str) -> StrResult<&'static Value> {
        self.scope().get(field).ok_or_else(|| {
            eco_format!("type `{}` does not contain field `{}`", self.short_name(), field)
        })
    }
}

// Type compatibility.
impl Type {
    /// The type's backward-compatible name.
    pub fn compat_name(&self) -> &str {
        self.long_name()
    }
}

#[scope]

Review comment (suggestion):
```suggestion
                eco_format!("type {self} does not have a constructor")
```

Code after:
        self.0.long_name
    }

    /// The type's title case name, for use in documentation (e.g. `String`).
    pub fn title(&self) -> &'static str {
        self.0.title
    }

    /// Documentation for the type (as Markdown).
    pub fn docs(&self) -> &'static str {
        self.0.docs
    }

    /// Search keywords for the type.
    pub fn keywords(&self) -> &'static [&'static str] {
        self.0.keywords
    }

    /// This type's constructor function.
    pub fn constructor(&self) -> StrResult<Func> {
        self.0
            .constructor
            .as_ref()
            .map(|lazy| Func::from(*lazy))
            .ok_or_else(|| eco_format!("type {self} does not have a constructor"))
    }

    /// The type's associated scope that holds sub-definitions.
    pub fn scope(&self) -> &'static Scope {
        &(self.0).0.scope
    }

    /// Get a field from this type's scope, if possible.
    pub fn field(&self, field: &str) -> StrResult<&'static Value> {
        self.scope()
            .get(field)
            .ok_or_else(|| eco_format!("type {self} does not contain field `{field}`"))
    }
}

// Type compatibility.
impl Type {
    /// The type's backward-compatible name.
    pub fn compat_name(&self) -> &str {
        self.long_name()
    }
}

#[scope]
impl Type {
    /// Determines a value's type.