ronantakizawa/codereview-bench huggingface dataset:
Repository: alacritty/alacritty
File: alacritty_terminal/src/ansi.rs (Rust)

Code before:
    fn parse_invalid_number() {
        assert_eq!(parse_number(b"1abc"), None);
    }

    #[test]
    fn parse_valid_number() {
        assert_eq!(parse_number(b"123"), Some(123));
    }

    #[test]
    fn parse_number_too_large() {
        assert_eq!(parse_number(b"321"), None);
    }

    #[test]
    fn parse_osc4_set_color() {
        let bytes: &[u8] = &[0x1b, b']', b'4', b';', b'0', b';', b'#', b'f', b'f', b'f', 0x1b];

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        assert_eq!(handler.color, xparse_color(&[b'#', b'f', b'f', b'f']));
    }
}


Review comment (suggestion):
Should use the Rgb here, not call `xparse_color` to verify the test result.

Code after:
    fn parse_invalid_number() {
        assert_eq!(parse_number(b"1abc"), None);
    }

    #[test]
    fn parse_valid_number() {
        assert_eq!(parse_number(b"123"), Some(123));
    }

    #[test]
    fn parse_number_too_large() {
        assert_eq!(parse_number(b"321"), None);
    }

    #[test]
    fn parse_osc4_set_color() {
        let bytes: &[u8] = b"\x1b]4;0;#fff\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        assert_eq!(handler.color, Some(Rgb { r: 0xf0, g: 0xf0, b: 0xf0 }));
    }
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/ansi.rs (Rust)

Code before:
    }

    #[test]
    fn parse_invalid_legacy_rgb_colors() {
        assert_eq!(xparse_color(b"#"), None);
        assert_eq!(xparse_color(b"#f"), None);
    }

    #[test]
    fn parse_invalid_number() {
        assert_eq!(parse_number(b"1abc"), None);
    }

    #[test]
    fn parse_valid_number() {
        assert_eq!(parse_number(b"123"), Some(123));
    }

    #[test]
    fn parse_number_too_large() {
        assert_eq!(parse_number(b"321"), None);
    }

    #[test]
    fn parse_osc4_set_color() {
        let bytes: &[u8] = &[0x1b, b']', b'4', b';', b'0', b';', b'#', b'f', b'f', b'f', 0x1b];

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        assert_eq!(handler.color, Some(Rgb { r: 0xf0, g: 0xf0, b: 0xf0 }));
    }
}


Review comment (security):
Could you please terminate this escape sequence properly? I don't like the idea of tests unrelated to error-handling relying on error-handling behavior. While `\e` is sufficient to terminate the escape, the proper sequence should be `\e\\` or `\a`.

Also the bytes specified should just be a byte string rather than an array, this is just unnecessarily verbose and difficult to read.

Code after:
    }

    #[test]
    fn parse_invalid_legacy_rgb_colors() {
        assert_eq!(xparse_color(b"#"), None);
        assert_eq!(xparse_color(b"#f"), None);
    }

    #[test]
    fn parse_invalid_number() {
        assert_eq!(parse_number(b"1abc"), None);
    }

    #[test]
    fn parse_valid_number() {
        assert_eq!(parse_number(b"123"), Some(123));
    }

    #[test]
    fn parse_number_too_large() {
        assert_eq!(parse_number(b"321"), None);
    }

    #[test]
    fn parse_osc4_set_color() {
        let bytes: &[u8] = b"\x1b]4;0;#fff\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        assert_eq!(handler.color, Some(Rgb { r: 0xf0, g: 0xf0, b: 0xf0 }));
    }
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/ansi.rs (Rust)

Code before:

    #[test]
    fn parse_osc104_reset_all_colors() {
        let bytes: &[u8] = b"\x1b]104;\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        let expected: Vec<usize> = (0..256).collect();
        assert_eq!(handler.reset_colors, expected);

        // And test without trailling semicolon
        let bytes: &[u8] = b"\x1b]104\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        assert_eq!(handler.reset_colors, expected);
    }
}


Review comment (suggestion):
Should be a separate test really. The two tests share nothing, so there's no reason to do it both in a single test.

Code after:

    #[test]
    fn parse_osc104_reset_all_colors() {
        let bytes: &[u8] = b"\x1b]104;\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        let expected: Vec<usize> = (0..256).collect();
        assert_eq!(handler.reset_colors, expected);
    }

    #[test]
    fn parse_osc104_reset_all_colors_no_semicolon() {
        let bytes: &[u8] = b"\x1b]104\x1b\\";

        let mut parser = Processor::new();
        let mut handler = MockHandler::default();

        for byte in bytes {
            parser.advance(&mut handler, *byte);
        }

        let expected: Vec<usize> = (0..256).collect();
        assert_eq!(handler.reset_colors, expected);
    }
}
Repository: alacritty/alacritty
File: alacritty/src/config/font.rs (Rust)

Code before:
    /// Get italic font description.
    pub fn italic(&self) -> FontDescription {
        self.italic.desc(&self.normal)
    }

    /// Get bold italic font description.
    pub fn bold_italic(&self) -> FontDescription {
        self.bold_italic.desc(&self.normal)
    }
}

impl Default for Font {
    fn default() -> Font {
        Self {
            offset: Default::default(),
            glyph_offset: Default::default(),
            use_thin_strokes: false,
            normal: Default::default(),
            bold: Default::default(),
            italic: Default::default(),
            bold_italic: Default::default(),
            size: Default::default(),
            builtin_box_drawing: true,
        }
    }
}

/// Description of the normal font.
#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct FontDescription {
    pub family: String,
    pub style: Option<String>,
}

impl Default for FontDescription {
    fn default() -> FontDescription {
        FontDescription {
            #[cfg(not(any(target_os = "macos", windows)))]
            family: "monospace".into(),
            #[cfg(target_os = "macos")]
            family: "Menlo".into(),
            #[cfg(windows)]
            family: "Consolas".into(),
            style: None,
        }
    }
}

/// Description of the italic and bold font.
#[derive(ConfigDeserialize, Debug, Default, Clone, PartialEq, Eq)]
pub struct SecondaryFontDescription {

Review comment (suggestion):
As an alternative, the option could be named `disable_builtin_box_drawing`, then the default of `false` wouldn't require this new impl.

Code after:
    /// Get italic font description.
    pub fn italic(&self) -> FontDescription {
        self.italic.desc(&self.normal)
    }

    /// Get bold italic font description.
    pub fn bold_italic(&self) -> FontDescription {
        self.bold_italic.desc(&self.normal)
    }
}

impl Default for Font {
    fn default() -> Font {
        Self {
            builtin_box_drawing: true,
            use_thin_strokes: Default::default(),
            glyph_offset: Default::default(),
            bold_italic: Default::default(),
            italic: Default::default(),
            offset: Default::default(),
            normal: Default::default(),
            bold: Default::default(),
            size: Default::default(),
        }
    }
}

/// Description of the normal font.
#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct FontDescription {
    pub family: String,
    pub style: Option<String>,
}

impl Default for FontDescription {
    fn default() -> FontDescription {
        FontDescription {
            #[cfg(not(any(target_os = "macos", windows)))]
            family: "monospace".into(),
            #[cfg(target_os = "macos")]
            family: "Menlo".into(),
            #[cfg(windows)]
            family: "Consolas".into(),
            style: None,
        }
    }
}

/// Description of the italic and bold font.
#[derive(ConfigDeserialize, Debug, Default, Clone, PartialEq, Eq)]
pub struct SecondaryFontDescription {
Repository: alacritty/alacritty
File: alacritty/src/config/font.rs (Rust)

Code before:
    pub fn normal(&self) -> &FontDescription {
        &self.normal
    }

    /// Get bold font description.
    pub fn bold(&self) -> FontDescription {
        self.bold.desc(&self.normal)
    }

    /// Get italic font description.
    pub fn italic(&self) -> FontDescription {
        self.italic.desc(&self.normal)
    }

    /// Get bold italic font description.
    pub fn bold_italic(&self) -> FontDescription {
        self.bold_italic.desc(&self.normal)
    }
}

impl Default for Font {
    fn default() -> Font {
        Self {
            builtin_box_drawing: true,
            use_thin_strokes: Default::default(),
            bold_italic: Default::default(),
            glyph_offset: Default::default(),
            italic: Default::default(),
            offset: Default::default(),
            normal: Default::default(),
            bold: Default::default(),
            size: Default::default(),
        }
    }
}

/// Description of the normal font.
#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct FontDescription {
    pub family: String,
    pub style: Option<String>,
}

impl Default for FontDescription {
    fn default() -> FontDescription {
        FontDescription {
            #[cfg(not(any(target_os = "macos", windows)))]
            family: "monospace".into(),
            #[cfg(target_os = "macos")]
            family: "Menlo".into(),
            #[cfg(windows)]

Review comment (suggestion):
```suggestion
            glyph_offset: Default::default(),
            bold_italic: Default::default(),
```

Code after:
    pub fn normal(&self) -> &FontDescription {
        &self.normal
    }

    /// Get bold font description.
    pub fn bold(&self) -> FontDescription {
        self.bold.desc(&self.normal)
    }

    /// Get italic font description.
    pub fn italic(&self) -> FontDescription {
        self.italic.desc(&self.normal)
    }

    /// Get bold italic font description.
    pub fn bold_italic(&self) -> FontDescription {
        self.bold_italic.desc(&self.normal)
    }
}

impl Default for Font {
    fn default() -> Font {
        Self {
            builtin_box_drawing: true,
            use_thin_strokes: Default::default(),
            glyph_offset: Default::default(),
            bold_italic: Default::default(),
            italic: Default::default(),
            offset: Default::default(),
            normal: Default::default(),
            bold: Default::default(),
            size: Default::default(),
        }
    }
}

/// Description of the normal font.
#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct FontDescription {
    pub family: String,
    pub style: Option<String>,
}

impl Default for FontDescription {
    fn default() -> FontDescription {
        FontDescription {
            #[cfg(not(any(target_os = "macos", windows)))]
            family: "monospace".into(),
            #[cfg(target_os = "macos")]
            family: "Menlo".into(),
            #[cfg(windows)]
Repository: alacritty/alacritty
File: alacritty_terminal/src/term/color.rs (Rust)

Code before:
use std::fmt::{self, Display, Formatter};
use std::ops::{Add, Index, IndexMut, Mul};
use std::str::FromStr;

use log::trace;
use serde::de::{Error as _, Visitor};
use serde::{Deserialize, Deserializer, Serialize};
use serde_yaml::Value;

use crate::ansi::NamedColor;

/// Number of terminal colors.
pub const COUNT: usize = 269;

/// Index of the cursor color.
pub const CURSOR_COLOR_INDEX: usize = 258;

#[derive(Debug, Eq, PartialEq, Copy, Clone, Default, Serialize)]
pub struct Rgb {
    pub r: u8,
    pub g: u8,
    pub b: u8,
}

impl Rgb {
    /// Implementation of W3C's luminance algorithm:
    /// https://www.w3.org/TR/WCAG20/#relativeluminancedef
    fn luminance(self) -> f64 {
        let channel_luminance = |channel| {
            let channel = channel as f64 / 255.;
            if channel <= 0.03928 {
                channel / 12.92
            } else {
                f64::powf((channel + 0.055) / 1.055, 2.4)
            }
        };

        let r_luminance = channel_luminance(self.r);
        let g_luminance = channel_luminance(self.g);
        let b_luminance = channel_luminance(self.b);


Review comment (suggestion):
You shouldn't use a constant. You should use `NamedColor::Cursor as usize`. Otherwise this will explode if we change the order of the colors.

Code after:
use std::fmt::{self, Display, Formatter};
use std::ops::{Add, Index, IndexMut, Mul};
use std::str::FromStr;

use log::trace;
use serde::de::{Error as _, Visitor};
use serde::{Deserialize, Deserializer, Serialize};
use serde_yaml::Value;

use crate::ansi::NamedColor;

/// Number of terminal colors.
pub const COUNT: usize = 269;

#[derive(Debug, Eq, PartialEq, Copy, Clone, Default, Serialize)]
pub struct Rgb {
    pub r: u8,
    pub g: u8,
    pub b: u8,
}

impl Rgb {
    /// Implementation of W3C's luminance algorithm:
    /// https://www.w3.org/TR/WCAG20/#relativeluminancedef
    fn luminance(self) -> f64 {
        let channel_luminance = |channel| {
            let channel = channel as f64 / 255.;
            if channel <= 0.03928 {
                channel / 12.92
            } else {
                f64::powf((channel + 0.055) / 1.055, 2.4)
            }
        };

        let r_luminance = channel_luminance(self.r);
        let g_luminance = channel_luminance(self.g);
        let b_luminance = channel_luminance(self.b);

        0.2126 * r_luminance + 0.7152 * g_luminance + 0.0722 * b_luminance
    }
Repository: alacritty/alacritty
File: alacritty_terminal/src/term/cell.rs (Rust)

Code before:
impl LineLength for grid::Row<Cell> {
    fn line_length(&self) -> Column {
        let mut length = Column(0);

        if self[Column(self.len() - 1)].flags.contains(Flags::WRAPLINE) {
            return Column(self.len());
        }

        for (index, cell) in self[..].iter().rev().enumerate() {
            if cell.c != ' '
                || cell.extra.as_ref().map(|extra| extra.zerowidth.is_empty()) == Some(false)
            {
                length = Column(self.len() - index);
                break;
            }
        }

        length
    }
}

#[cfg(test)]
mod tests {
    use std::mem;

    use super::{Cell, LineLength};

    use crate::grid::Row;
    use crate::index::Column;

    #[test]
    fn cell_size_is_below_cap() {
        // Estimated cell size on 64-bit architectures.
        const ESTIMATED_CELL_SIZE: usize = 24;

        // Ensure that cell size isn't growning by accident.
        assert!(mem::size_of::<Cell>() <= ESTIMATED_CELL_SIZE);
    }

    #[test]
    fn line_length_works() {
        let mut row = Row::<Cell>::new(10);
        row[Column(5)].c = 'a';

        assert_eq!(row.line_length(), Column(6));
    }

    #[test]
    fn line_length_works_with_wrapline() {
        let mut row = Row::<Cell>::new(10);
        row[Column(9)].flags.insert(super::Flags::WRAPLINE);

Review comment (suggestion):
```suggestion
    use super::*;

    use std::mem;
```

I think this would be more consistent with the rest of our code.

Code after:
                length = Column(self.len() - index);
                break;
            }
        }

        length
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    use std::mem;

    use crate::grid::Row;
    use crate::index::Column;

    #[test]
    fn cell_size_is_below_cap() {
        // Expected cell size on 64-bit architectures.
        const EXPECTED_CELL_SIZE: usize = 24;

        // Ensure that cell size isn't growning by accident.
        assert!(mem::size_of::<Cell>() <= EXPECTED_CELL_SIZE);
    }

    #[test]
    fn line_length_works() {
        let mut row = Row::<Cell>::new(10);
        row[Column(5)].c = 'a';

        assert_eq!(row.line_length(), Column(6));
    }

    #[test]
    fn line_length_works_with_wrapline() {
        let mut row = Row::<Cell>::new(10);
        row[Column(9)].flags.insert(super::Flags::WRAPLINE);

        assert_eq!(row.line_length(), Column(10));
    }
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/term/cell.rs (Rust)

Code before:
        for (index, cell) in self[..].iter().rev().enumerate() {
            if cell.c != ' '
                || cell.extra.as_ref().map(|extra| extra.zerowidth.is_empty()) == Some(false)
            {
                length = Column(self.len() - index);
                break;
            }
        }

        length
    }
}

#[cfg(test)]
mod tests {
    use std::mem;

    use super::{Cell, LineLength};

    use crate::grid::Row;
    use crate::index::Column;

    #[test]
    fn cell_size_is_below_cap() {
        // Estimated cell size on 64-bit architectures.
        const ESTIMATED_CELL_SIZE: usize = 24;

        // Ensure that cell size isn't growning by accident.
        assert!(mem::size_of::<Cell>() <= ESTIMATED_CELL_SIZE);
    }

    #[test]
    fn line_length_works() {
        let mut row = Row::<Cell>::new(10);
        row[Column(5)].c = 'a';

        assert_eq!(row.line_length(), Column(6));
    }

    #[test]
    fn line_length_works_with_wrapline() {
        let mut row = Row::<Cell>::new(10);
        row[Column(9)].flags.insert(super::Flags::WRAPLINE);

        assert_eq!(row.line_length(), Column(10));
    }
}


Review comment (suggestion):
```suggestion
        // Expected cell size on 64-bit architectures.
        const EXPECTED_MAX_CELL_SIZE: usize = 24;
```

I think `expected` is a bit better than `estimated` here. We aren't really guessing at a value, but we **know** it should be under this exact number.

Code after:

#[cfg(test)]
mod tests {
    use super::*;

    use std::mem;

    use crate::grid::Row;
    use crate::index::Column;

    #[test]
    fn cell_size_is_below_cap() {
        // Expected cell size on 64-bit architectures.
        const EXPECTED_CELL_SIZE: usize = 24;

        // Ensure that cell size isn't growning by accident.
        assert!(mem::size_of::<Cell>() <= EXPECTED_CELL_SIZE);
    }

    #[test]
    fn line_length_works() {
        let mut row = Row::<Cell>::new(10);
        row[Column(5)].c = 'a';

        assert_eq!(row.line_length(), Column(6));
    }

    #[test]
    fn line_length_works_with_wrapline() {
        let mut row = Row::<Cell>::new(10);
        row[Column(9)].flags.insert(super::Flags::WRAPLINE);

        assert_eq!(row.line_length(), Column(10));
    }
}
Repository: alacritty/alacritty
File: alacritty/src/renderer/text/glsl3.rs (Rust)

Code before:
use std::mem::size_of;
use std::ptr;

use crossfont::RasterizedGlyph;
use log::info;

use alacritty_terminal::term::cell::Flags;

use crate::display::content::RenderableCell;
use crate::display::SizeInfo;
use crate::gl;
use crate::gl::types::*;
use crate::renderer::shader::{ShaderProgram, ShaderVersion};
use crate::renderer::{cstr, Error};

use super::atlas::{Atlas, ATLAS_SIZE};
use super::{
    Glyph, LoadGlyph, LoaderApi, RenderingGlyphFlags, RenderingPass, TextRenderApi,
    TextRenderBatch, TextRenderer, TextShader,
};

// Shader source.
pub(super) static TEXT_SHADER_F: &str = include_str!("../../../res/glsl3/text.f.glsl");
static TEXT_SHADER_V: &str = include_str!("../../../res/glsl3/text.v.glsl");

/// Maximum items to be drawn in a batch.
const BATCH_MAX: usize = 0x1_0000;

#[derive(Debug)]
pub struct Glsl3Renderer {
    program: TextShaderProgram,
    vao: GLuint,
    ebo: GLuint,
    vbo_instance: GLuint,
    atlas: Vec<Atlas>,
    current_atlas: usize,
    active_tex: GLuint,
    batch: Batch,
}

impl Glsl3Renderer {
    pub fn new() -> Result<Self, Error> {
        info!("Using OpenGL 3.3 renderer");

        let program = TextShaderProgram::new(ShaderVersion::Glsl3)?;
        let mut vao: GLuint = 0;
        let mut ebo: GLuint = 0;
        let mut vbo_instance: GLuint = 0;

Review comment (suggestion):
Do not use `pub(super)` on anything please. Either `pub`, `pub(crate)`, or no pub at all. Using `pub(super)` is just unnecessarily convoluted. For our purposes, Alacritty being our binary anyway, we should be using `pub`.

Code after:
use std::mem::size_of;
use std::ptr;

use crossfont::RasterizedGlyph;
use log::info;

use alacritty_terminal::term::cell::Flags;

use crate::display::content::RenderableCell;
use crate::display::SizeInfo;
use crate::gl;
use crate::gl::types::*;
use crate::renderer::shader::{ShaderProgram, ShaderVersion};
use crate::renderer::{cstr, Error};

use super::atlas::{Atlas, ATLAS_SIZE};
use super::{
    Glyph, LoadGlyph, LoaderApi, RenderingGlyphFlags, RenderingPass, TextRenderApi,
    TextRenderBatch, TextRenderer, TextShader,
};

// Shader source.
pub static TEXT_SHADER_F: &str = include_str!("../../../res/glsl3/text.f.glsl");
static TEXT_SHADER_V: &str = include_str!("../../../res/glsl3/text.v.glsl");

/// Maximum items to be drawn in a batch.
const BATCH_MAX: usize = 0x1_0000;

#[derive(Debug)]
pub struct Glsl3Renderer {
    program: TextShaderProgram,
    vao: GLuint,
    ebo: GLuint,
    vbo_instance: GLuint,
    atlas: Vec<Atlas>,
    current_atlas: usize,
    active_tex: GLuint,
    batch: Batch,
}

impl Glsl3Renderer {
    pub fn new() -> Result<Self, Error> {
        info!("Using OpenGL 3.3 renderer");

        let program = TextShaderProgram::new(ShaderVersion::Glsl3)?;
        let mut vao: GLuint = 0;
        let mut ebo: GLuint = 0;
        let mut vbo_instance: GLuint = 0;