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

Code before:
    pub vi_highlighted_hint: Option<HintMatch>,

    pub is_wayland: bool,

    /// UI cursor visibility for blinking.
    pub cursor_hidden: bool,

    pub visual_bell: VisualBell,

    /// Mapped RGB values for each terminal color.
    pub colors: List,

    /// State of the keyboard hints.
    pub hint_state: HintState,

    /// Unprocessed display updates.
    pub pending_update: DisplayUpdate,

    /// The renderer update that takes place only once before the actual rendering.
    pub pending_renderer_update: Option<RendererUpdate>,

    /// The ime on the given display.
    pub ime: Ime,

    /// The state of the timer for frame scheduling.
    pub frame_timer_info: FrameTimerInfo,

    // Mouse point position when highlighting hints.
    hint_mouse_point: Option<Point>,

    renderer: ManuallyDrop<Renderer>,

    surface: ManuallyDrop<Surface<WindowSurface>>,

    context: ManuallyDrop<Replaceable<PossiblyCurrentContext>>,

    debug_damage: bool,
    damage_rects: Vec<DamageRect>,
    next_frame_damage_rects: Vec<DamageRect>,
    glyph_cache: GlyphCache,
    meter: Meter,
}

impl Display {
    pub fn new(
        window: Window,
        gl_context: NotCurrentContext,
        config: &UiConfig,
    ) -> Result<Display, Error> {
        #[cfg(any(not(feature = "wayland"), target_os = "macos", windows))]
        let is_wayland = false;

Review comment (suggestion):
```suggestion
    pub frame_timer: FrameTimer,
```

"Info" is a pretty useless suffix usually, I think we can just remove it.

Code after:
    pub vi_highlighted_hint: Option<HintMatch>,

    pub is_wayland: bool,

    /// UI cursor visibility for blinking.
    pub cursor_hidden: bool,

    pub visual_bell: VisualBell,

    /// Mapped RGB values for each terminal color.
    pub colors: List,

    /// State of the keyboard hints.
    pub hint_state: HintState,

    /// Unprocessed display updates.
    pub pending_update: DisplayUpdate,

    /// The renderer update that takes place only once before the actual rendering.
    pub pending_renderer_update: Option<RendererUpdate>,

    /// The ime on the given display.
    pub ime: Ime,

    /// The state of the timer for frame scheduling.
    pub frame_timer: FrameTimer,

    // Mouse point position when highlighting hints.
    hint_mouse_point: Option<Point>,

    renderer: ManuallyDrop<Renderer>,

    surface: ManuallyDrop<Surface<WindowSurface>>,

    context: ManuallyDrop<Replaceable<PossiblyCurrentContext>>,

    debug_damage: bool,
    damage_rects: Vec<DamageRect>,
    next_frame_damage_rects: Vec<DamageRect>,
    glyph_cache: GlyphCache,
    meter: Meter,
}

impl Display {
    pub fn new(
        window: Window,
        gl_context: NotCurrentContext,
        config: &UiConfig,
    ) -> Result<Display, Error> {
        #[cfg(any(not(feature = "wayland"), target_os = "macos", windows))]
        let is_wayland = false;
Repository: alacritty/alacritty
File: alacritty/src/display/window.rs (Rust)

Code before:
        let atom = (xlib.XInternAtom)(xlib_display as *mut _, "_XEMBED".as_ptr() as *const _, 0);
        (xlib.XChangeProperty)(
            xlib_display as _,
            xlib_window as _,
            atom,
            atom,
            32,
            PropModeReplace,
            [0, 1].as_ptr(),
            2,
        );

        // Register new error handler.
        let old_handler = (xlib.XSetErrorHandler)(Some(xembed_error_handler));

        // Check for the existence of the target before attempting reparenting.
        (xlib.XReparentWindow)(xlib_display as _, xlib_window as _, parent_id, 0, 0);

        // Drain errors and restore original error handler.
        (xlib.XSync)(xlib_display as _, 0);
        (xlib.XSetErrorHandler)(old_handler);
    }
}

#[cfg(target_os = "macos")]
fn macos_use_srgb_color_space(window: &WinitWindow) {
    let raw_window = match window.raw_window_handle() {
        RawWindowHandle::AppKit(handle) => handle.ns_window as id,
        _ => return,
    };

    unsafe {
        let _: () = msg_send![raw_window, setColorSpace: NSColorSpace::sRGBColorSpace(nil)];
    }
}

#[cfg(all(feature = "x11", not(any(target_os = "macos", windows))))]
unsafe extern "C" fn xembed_error_handler(_: *mut XDisplay, _: *mut XErrorEvent) -> i32 {
    log::error!("Could not embed into specified window.");
    std::process::exit(1);
}


Review comment (suggestion):
```suggestion
fn use_srgb_color_space(window: &WinitWindow) {
```

Code after:
        let atom = (xlib.XInternAtom)(xlib_display as *mut _, "_XEMBED".as_ptr() as *const _, 0);
        (xlib.XChangeProperty)(
            xlib_display as _,
            xlib_window as _,
            atom,
            atom,
            32,
            PropModeReplace,
            [0, 1].as_ptr(),
            2,
        );

        // Register new error handler.
        let old_handler = (xlib.XSetErrorHandler)(Some(xembed_error_handler));

        // Check for the existence of the target before attempting reparenting.
        (xlib.XReparentWindow)(xlib_display as _, xlib_window as _, parent_id, 0, 0);

        // Drain errors and restore original error handler.
        (xlib.XSync)(xlib_display as _, 0);
        (xlib.XSetErrorHandler)(old_handler);
    }
}

#[cfg(target_os = "macos")]
fn use_srgb_color_space(window: &WinitWindow) {
    let raw_window = match window.raw_window_handle() {
        RawWindowHandle::AppKit(handle) => handle.ns_window as id,
        _ => return,
    };

    unsafe {
        let _: () = msg_send![raw_window, setColorSpace: NSColorSpace::sRGBColorSpace(nil)];
    }
}

#[cfg(all(feature = "x11", not(any(target_os = "macos", windows))))]
unsafe extern "C" fn xembed_error_handler(_: *mut XDisplay, _: *mut XErrorEvent) -> i32 {
    log::error!("Could not embed into specified window.");
    std::process::exit(1);
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/tty/unix.rs (Rust)

Code before:
impl Pty {
    pub fn child(&self) -> &Child {
        &self.child
    }

    pub fn file(&self) -> &File {
        &self.file
    }
}

struct ShellUser {
    user: String,
    longname: String,
    home: String,
    shell: String,
}

impl ShellUser {
    /// look for shell, username, longname, and home dir in the respective environment variables
    /// before falling back on looking in to `passwd`.
    fn from_env() -> Result<Self> {
        let mut buf = [0; 1024];
        let pw = get_pw_entry(&mut buf);

        let user = match env::var("USER") {
            Ok(u) => u,
            Err(_) => match pw {
                Ok(ref pw) => pw.name.to_owned(),
                Err(err) => return Err(err),
            },
        };

        let home = match env::var("HOME") {
            Ok(h) => h,
            Err(_) => match pw {
                Ok(ref pw) => pw.dir.to_owned(),
                Err(err) => return Err(err),
            },
        };

        let shell = match env::var("SHELL") {
            Ok(s) => s,
            Err(_) => match pw {
                Ok(ref pw) => pw.shell.to_owned(),
                Err(err) => return Err(err),
            },
        };

        let longname = std::env::var("LONGNAME").unwrap_or_else(|_| user.to_owned());

        Ok(Self { user, longname, home, shell })

Review comment (suggestion):
Please refrain from single-character variables. There's no reason not to just use `user` here. Same applies to the other match blocks.

Code after:
impl Pty {
    pub fn child(&self) -> &Child {
        &self.child
    }

    pub fn file(&self) -> &File {
        &self.file
    }
}

/// User information that is required for a new shell session.
struct ShellUser {
    user: String,
    home: String,
    shell: String,
}

impl ShellUser {
    /// look for shell, username, longname, and home dir in the respective environment variables
    /// before falling back on looking in to `passwd`.
    fn from_env() -> Result<Self> {
        let mut buf = [0; 1024];
        let pw = get_pw_entry(&mut buf);

        let user = match env::var("USER") {
            Ok(user) => user,
            Err(_) => match pw {
                Ok(ref pw) => pw.name.to_owned(),
                Err(err) => return Err(err),
            },
        };

        let home = match env::var("HOME") {
            Ok(home) => home,
            Err(_) => match pw {
                Ok(ref pw) => pw.dir.to_owned(),
                Err(err) => return Err(err),
            },
        };

        let shell = match env::var("SHELL") {
            Ok(shell) => shell,
            Err(_) => match pw {
                Ok(ref pw) => pw.shell.to_owned(),
                Err(err) => return Err(err),
            },
        };

        Ok(Self { user, home, shell })
    }
}
Repository: alacritty/alacritty
File: alacritty/src/config/window.rs (Rust)

Code before:
        let padding_x = (f32::from(self.padding.x) * scale_factor).floor();
        let padding_y = (f32::from(self.padding.y) * scale_factor).floor();
        (padding_x, padding_y)
    }

    #[inline]
    pub fn fullscreen(&self) -> Option<Fullscreen> {
        if self.startup_mode == StartupMode::Fullscreen {
            Some(Fullscreen::Borderless(None))
        } else {
            None
        }
    }

    #[inline]
    pub fn maximized(&self) -> bool {
        self.startup_mode == StartupMode::Maximized
    }

    #[cfg(target_os = "macos")]
    pub fn option_as_alt(&self) -> winit::platform::macos::OptionAsAlt {
        match self.option_as_alt {
            OptionAsAlt::OnlyLeft => winit::platform::macos::OptionAsAlt::OnlyLeft,
            OptionAsAlt::OnlyRight => winit::platform::macos::OptionAsAlt::OnlyRight,
            OptionAsAlt::Both => winit::platform::macos::OptionAsAlt::Both,
            OptionAsAlt::None => winit::platform::macos::OptionAsAlt::None,
        }
    }
}

#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct Identity {
    /// Window title.
    pub title: String,

    /// Window class.
    pub class: Class,
}

impl Default for Identity {
    fn default() -> Self {
        Self { title: DEFAULT_NAME.into(), class: Default::default() }
    }
}

#[derive(ConfigDeserialize, Default, Debug, Copy, Clone, PartialEq, Eq)]
pub enum StartupMode {
    #[default]
    Windowed,
    Maximized,
    Fullscreen,

Review comment (suggestion):
Shouldn't use the full path here. Just import it as `WinitOptionsAsAlt` or something.

Code after:
        let padding_x = (f32::from(self.padding.x) * scale_factor).floor();
        let padding_y = (f32::from(self.padding.y) * scale_factor).floor();
        (padding_x, padding_y)
    }

    #[inline]
    pub fn fullscreen(&self) -> Option<Fullscreen> {
        if self.startup_mode == StartupMode::Fullscreen {
            Some(Fullscreen::Borderless(None))
        } else {
            None
        }
    }

    #[inline]
    pub fn maximized(&self) -> bool {
        self.startup_mode == StartupMode::Maximized
    }

    #[cfg(target_os = "macos")]
    pub fn option_as_alt(&self) -> WinitOptionAsAlt {
        match self.option_as_alt {
            OptionAsAlt::OnlyLeft => WinitOptionAsAlt::OnlyLeft,
            OptionAsAlt::OnlyRight => WinitOptionAsAlt::OnlyRight,
            OptionAsAlt::Both => WinitOptionAsAlt::Both,
            OptionAsAlt::None => WinitOptionAsAlt::None,
        }
    }
}

#[derive(ConfigDeserialize, Debug, Clone, PartialEq, Eq)]
pub struct Identity {
    /// Window title.
    pub title: String,

    /// Window class.
    pub class: Class,
}

impl Default for Identity {
    fn default() -> Self {
        Self { title: DEFAULT_NAME.into(), class: Default::default() }
    }
}

#[derive(ConfigDeserialize, Default, Debug, Copy, Clone, PartialEq, Eq)]
pub enum StartupMode {
    #[default]
    Windowed,
    Maximized,
    Fullscreen,
Repository: alacritty/alacritty
File: alacritty_terminal/src/config/mod.rs (Rust)

Code before:
        }
    }
}

impl From<CursorBlinking> for bool {
    fn from(blinking: CursorBlinking) -> bool {
        blinking == CursorBlinking::On || blinking == CursorBlinking::Always
    }
}

#[derive(ConfigDeserialize, Default, Debug, Clone, Copy, PartialEq, Eq)]
pub enum OptionAsAlt {
    /// The left `Option` key is treated as `Alt`.
    OnlyLeft,

    /// The right `Option` key is treated as `Alt`.
    OnlyRight,

    /// Both `Option` keys are treated as `Alt`.
    Both,

    /// No special handling is applied for `Option` key.
    #[default]
    None,
}

#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
#[serde(untagged, deny_unknown_fields)]
pub enum Program {
    Just(String),
    WithArgs {
        program: String,
        #[serde(default)]
        args: Vec<String>,
    },
}

impl Program {
    pub fn program(&self) -> &str {
        match self {
            Program::Just(program) => program,
            Program::WithArgs { program, .. } => program,
        }
    }

    pub fn args(&self) -> &[String] {
        match self {
            Program::Just(_) => &[],
            Program::WithArgs { args, .. } => args,
        }
    }

Review comment (suggestion):
This is only used in alacritty, the type has no use in `alacritty_terminal`, because it doesn't do anything related to keyboard input, just simply move it into `alacritty`.

Code after:
        }
    }
}

impl From<CursorBlinking> for bool {
    fn from(blinking: CursorBlinking) -> bool {
        blinking == CursorBlinking::On || blinking == CursorBlinking::Always
    }
}

#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
#[serde(untagged, deny_unknown_fields)]
pub enum Program {
    Just(String),
    WithArgs {
        program: String,
        #[serde(default)]
        args: Vec<String>,
    },
}

impl Program {
    pub fn program(&self) -> &str {
        match self {
            Program::Just(program) => program,
            Program::WithArgs { program, .. } => program,
        }
    }

    pub fn args(&self) -> &[String] {
        match self {
            Program::Just(_) => &[],
            Program::WithArgs { args, .. } => args,
        }
    }
}

/// Wrapper around f32 that represents a percentage value between 0.0 and 1.0.
#[derive(SerdeReplace, Deserialize, Clone, Copy, Debug, PartialEq)]
pub struct Percentage(f32);

impl Default for Percentage {
    fn default() -> Self {
        Percentage(1.0)
    }
}

impl Percentage {
    pub fn new(value: f32) -> Self {
        Percentage(value.clamp(0., 1.))
    }
Repository: alacritty/alacritty
File: alacritty/src/main.rs (Rust)

Code before:
    //     of them own an Arc<ConPTY>
    //
    // The fix is to ensure that processor is dropped first. That way, when window context (i.e.
    // PTY) is dropped, it can ensure ConPTY is dropped before the conout pipe in the PTY drop
    // order.
    //
    // FIXME: Change PTY API to enforce the correct drop order with the typesystem.
    drop(processor);

    // FIXME patch notify library to have a shutdown method.
    // config_reloader.join().ok();

    // Error reporting on Windows.
    // If an error has occurred, show it in on the console and in a message box.
    // It is done before FreeConsole() so the error appears on the console.
    #[cfg(windows)]
    if let Err(ref err) = result {
        eprintln!("Error: {}", err);
        if let Some(source) = err.source() {
            eprintln!("Source: {}", source);
        }
        panic::error_message_box(&err.to_string(), "An error has occurred");
    }

    // Without explicitly detaching the console cmd won't redraw its prompt.
    #[cfg(windows)]
    unsafe {
        FreeConsole();
    }

    info!("Goodbye");

    result
}

fn log_config_path(config: &UiConfig) {
    if config.config_paths.is_empty() {
        return;
    }

    let mut msg = String::from("Configuration files loaded from:");
    for path in &config.config_paths {
        let _ = write!(msg, "\n  {:?}", path.display());
    }

    info!("{}", msg);
}


Review comment (suggestion):
The point was to consistently show this message box for all errors encountered, so it should be not here, but in `fn main` where the all errors are being returned, though, I'd assume only for `alacritty` itself we need a message box.

Code after:
    // The cause:
    //   - Drop for ConPTY will deadlock if the conout pipe has already been dropped
    //   - ConPTY is dropped when the last of processor and window context are dropped, because both
    //     of them own an Arc<ConPTY>
    //
    // The fix is to ensure that processor is dropped first. That way, when window context (i.e.
    // PTY) is dropped, it can ensure ConPTY is dropped before the conout pipe in the PTY drop
    // order.
    //
    // FIXME: Change PTY API to enforce the correct drop order with the typesystem.
    drop(processor);

    // FIXME patch notify library to have a shutdown method.
    // config_reloader.join().ok();

    // Without explicitly detaching the console cmd won't redraw it's prompt.
    #[cfg(windows)]
    unsafe {
        FreeConsole();
    }

    info!("Goodbye");

    result
}

fn log_config_path(config: &UiConfig) {
    if config.config_paths.is_empty() {
        return;
    }

    let mut msg = String::from("Configuration files loaded from:");
    for path in &config.config_paths {
        let _ = write!(msg, "\n  {:?}", path.display());
    }

    info!("{}", msg);
}
Repository: alacritty/alacritty
File: alacritty/src/renderer/text/builtin_font.rs (Rust)

Code before:
//! Hand-rolled drawing of unicode [box drawing](http://www.unicode.org/charts/PDF/U2500.pdf),
//! [block elements](https://www.unicode.org/charts/PDF/U2580.pdf)
//! and [sextants](https://www.unicode.org/charts/PDF/U1FB00.pdf), and also powerline symbols.

use std::{cmp, mem, ops};

use crossfont::{BitmapBuffer, Metrics, RasterizedGlyph};

use crate::config::ui_config::Delta;

// Colors which are used for filling shade variants.
const COLOR_FILL_ALPHA_STEP_1: Pixel = Pixel { _r: 192, _g: 192, _b: 192 };
const COLOR_FILL_ALPHA_STEP_2: Pixel = Pixel { _r: 128, _g: 128, _b: 128 };
const COLOR_FILL_ALPHA_STEP_3: Pixel = Pixel { _r: 64, _g: 64, _b: 64 };

/// Default color used for filling.
const COLOR_FILL: Pixel = Pixel { _r: 255, _g: 255, _b: 255 };

const POWERLINE_TRIANGLE_LTR: char = '\u{e0b0}';
const POWERLINE_ARROW_LTR: char = '\u{e0b1}';
const POWERLINE_TRIANGLE_RTL: char = '\u{e0b2}';
const POWERLINE_ARROW_RTL: char = '\u{e0b3}';

/// Returns the rasterized glyph if the character is part of the built-in font.
pub fn builtin_glyph(
    character: char,
    metrics: &Metrics,
    offset: &Delta<i8>,

Review comment (suggestion):
This comment should be reworded to not include what exactly is being drawn. Otherwise we'll have to change it constantly.

Code after:
//! Hand-rolled drawing of unicode characters that need to fully cover their character area.

use std::{cmp, mem, ops};

use crossfont::{BitmapBuffer, Metrics, RasterizedGlyph};

use crate::config::ui_config::Delta;

// Colors which are used for filling shade variants.
const COLOR_FILL_ALPHA_STEP_1: Pixel = Pixel { _r: 192, _g: 192, _b: 192 };
const COLOR_FILL_ALPHA_STEP_2: Pixel = Pixel { _r: 128, _g: 128, _b: 128 };
const COLOR_FILL_ALPHA_STEP_3: Pixel = Pixel { _r: 64, _g: 64, _b: 64 };

/// Default color used for filling.
const COLOR_FILL: Pixel = Pixel { _r: 255, _g: 255, _b: 255 };

const POWERLINE_TRIANGLE_LTR: char = '\u{e0b0}';
const POWERLINE_ARROW_LTR: char = '\u{e0b1}';
const POWERLINE_TRIANGLE_RTL: char = '\u{e0b2}';
const POWERLINE_ARROW_RTL: char = '\u{e0b3}';

/// Returns the rasterized glyph if the character is part of the built-in font.
pub fn builtin_glyph(
    character: char,
    metrics: &Metrics,
    offset: &Delta<i8>,
    glyph_offset: &Delta<i8>,
) -> Option<RasterizedGlyph> {
Repository: alacritty/alacritty
File: alacritty/src/renderer/text/builtin_font.rs (Rust)

Code before:
//! Hand-rolled drawing of unicode [box drawing](http://www.unicode.org/charts/PDF/U2500.pdf),
//! [block elements](https://www.unicode.org/charts/PDF/U2580.pdf)
//! and [symbols for legacy computing](https://www.unicode.org/charts/PDF/U1FB00.pdf),
//! and also powerline symbols.

use std::{cmp, mem, ops};

use crossfont::{BitmapBuffer, Metrics, RasterizedGlyph};

use crate::config::ui_config::Delta;

// Colors which are used for filling shade variants.
const COLOR_FILL_ALPHA_STEP_1: Pixel = Pixel { _r: 192, _g: 192, _b: 192 };
const COLOR_FILL_ALPHA_STEP_2: Pixel = Pixel { _r: 128, _g: 128, _b: 128 };
const COLOR_FILL_ALPHA_STEP_3: Pixel = Pixel { _r: 64, _g: 64, _b: 64 };

/// Default color used for filling.
const COLOR_FILL: Pixel = Pixel { _r: 255, _g: 255, _b: 255 };

const POWERLINE_TRIANGLE_LTR: char = '\u{e0b0}';
const POWERLINE_ARROW_LTR: char = '\u{e0b1}';
const POWERLINE_TRIANGLE_RTL: char = '\u{e0b2}';
const POWERLINE_ARROW_RTL: char = '\u{e0b3}';

/// Returns the rasterized glyph if the character is part of the built-in font.
pub fn builtin_glyph(
    character: char,
    metrics: &Metrics,
    offset: &Delta<i8>,

Review comment (suggestion):
As stated in my last review:

> This comment should be reworded to not include what exactly is being drawn. Otherwise we'll have to change it constantly.

I'd appreciate not resolving reviews that haven't been completed.

Code after:
//! Hand-rolled drawing of unicode characters that need to fully cover their character area.

use std::{cmp, mem, ops};

use crossfont::{BitmapBuffer, Metrics, RasterizedGlyph};

use crate::config::ui_config::Delta;

// Colors which are used for filling shade variants.
const COLOR_FILL_ALPHA_STEP_1: Pixel = Pixel { _r: 192, _g: 192, _b: 192 };
const COLOR_FILL_ALPHA_STEP_2: Pixel = Pixel { _r: 128, _g: 128, _b: 128 };
const COLOR_FILL_ALPHA_STEP_3: Pixel = Pixel { _r: 64, _g: 64, _b: 64 };

/// Default color used for filling.
const COLOR_FILL: Pixel = Pixel { _r: 255, _g: 255, _b: 255 };

const POWERLINE_TRIANGLE_LTR: char = '\u{e0b0}';
const POWERLINE_ARROW_LTR: char = '\u{e0b1}';
const POWERLINE_TRIANGLE_RTL: char = '\u{e0b2}';
const POWERLINE_ARROW_RTL: char = '\u{e0b3}';

/// Returns the rasterized glyph if the character is part of the built-in font.
pub fn builtin_glyph(
    character: char,
    metrics: &Metrics,
    offset: &Delta<i8>,
    glyph_offset: &Delta<i8>,
) -> Option<RasterizedGlyph> {
    let mut glyph = match character {