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

Code before:
            if extensions.is_null() {
                // We're on the core profile, query extension one by one.
                Self::from_core_profile()
            } else {
                Self::from_compatible_profile()
            }
        }
    }

    unsafe fn from_core_profile() -> Self {
        let mut extensions_number = 0;
        gl::GetIntegerv(gl::NUM_EXTENSIONS, &mut extensions_number);

        let extensions = (0..extensions_number as gl::types::GLuint).map(|i| {
            let extension = CStr::from_ptr(gl::GetStringi(gl::EXTENSIONS, i) as *mut _);
            extension.to_str()
        });

        GlExtensions { extensions: Box::new(extensions) }
    }

    unsafe fn from_compatible_profile() -> Self {
        // SAFETY: OpenGL returns a pointer to a `static` string, so using static lifetime.
        let extensions = gl::GetString(gl::EXTENSIONS);
        let extensions: Box<dyn Iterator<Item = Result<&'static str, Utf8Error>>> =
            match std::mem::transmute::<Result<&'_ str, Utf8Error>, Result<&'static str, Utf8Error>>(
                CStr::from_ptr(extensions as *mut _).to_str(),
            ) {
                Ok(ext) => Box::new(ext.split_whitespace().map(Ok)),
                Err(err) => Box::new(std::iter::once(Err(err))),
            };

        GlExtensions { extensions }
    }
}

impl Iterator for GlExtensions {
    type Item = Result<&'static str, Utf8Error>;

    fn next(&mut self) -> Option<Self::Item> {
        self.extensions.next()
    }
}


Review comment (suggestion):
This looks **very** unsafe. Are you just using `transmute` to extend the lifetime here? Because you should **definitely** not do that.

Code after:
        static OPENGL_EXTENSIONS: OnceCell<HashSet<&'static str>> = OnceCell::new();

        OPENGL_EXTENSIONS.get_or_init(Self::load_extensions).contains(extension)
    }

    /// Load available OpenGL extensions.
    fn load_extensions() -> HashSet<&'static str> {
        unsafe {
            let extensions = gl::GetString(gl::EXTENSIONS);

            if extensions.is_null() {
                let mut extensions_number = 0;
                gl::GetIntegerv(gl::NUM_EXTENSIONS, &mut extensions_number);

                (0..extensions_number as gl::types::GLuint)
                    .flat_map(|i| {
                        let extension = CStr::from_ptr(gl::GetStringi(gl::EXTENSIONS, i) as *mut _);
                        extension.to_str()
                    })
                    .collect()
            } else {
                match CStr::from_ptr(extensions as *mut _).to_str() {
                    Ok(ext) => ext.split_whitespace().collect(),
                    Err(_) => HashSet::new(),
                }
            }
        }
    }
}
Repository: alacritty/alacritty
File: alacritty/src/renderer/text/mod.rs (Rust)

Code before:
use crate::display::SizeInfo;
use crate::gl;
use crate::gl::types::*;

mod atlas;
mod builtin_font;
mod gles2;
mod glsl3;
pub mod glyph_cache;

use atlas::Atlas;
pub use gles2::Gles2Renderer;
pub use glsl3::Glsl3Renderer;
pub use glyph_cache::GlyphCache;
use glyph_cache::{Glyph, LoadGlyph};

// NOTE: These flags must be in sync with their usage in the text.*.glsl shaders.
bitflags! {
    #[repr(C)]
    struct RenderingGlyphFlags: u8 {
        const COLORED   = 0b0000_0001;
        const WIDE_CHAR = 0b0000_0010;
    }
}

// NOTE: the passes are shared between GLES2 and GLSL3 renderers.
#[repr(u8)]
enum RenderingPass {
    /// Rendering pass used to render background color in text shaders.
    Background = 0,

    /// The first pass to render text with both GLES2 and GLSL3 renderers.
    SubpixelPass1 = 1,

    /// The second pass to render text with GLES2 renderer.
    SubpixelPass2 = 2,

    /// The third pass to render text with GLES2 renderer.
    SubpixelPass3 = 3,
}

pub trait TextRenderer<'a> {
    type Shader: TextShader;
    type RenderBatch: TextRenderBatch;
    type RenderApi: TextRenderApi<Self::RenderBatch>;

    /// Get loader API for the renderer.
    fn loader_api(&mut self) -> LoaderApi<'_>;

    /// Draw cells.
    fn draw_cells<'b: 'a, I: Iterator<Item = RenderableCell>>(

Review comment (suggestion):
```suggestion
/// Rendering passes, for both GLES2 and GLSL3 renderer.
```

Code after:
use crate::display::SizeInfo;
use crate::gl;
use crate::gl::types::*;

mod atlas;
mod builtin_font;
mod gles2;
mod glsl3;
pub mod glyph_cache;

use atlas::Atlas;
pub use gles2::Gles2Renderer;
pub use glsl3::Glsl3Renderer;
pub use glyph_cache::GlyphCache;
use glyph_cache::{Glyph, LoadGlyph};

// NOTE: These flags must be in sync with their usage in the text.*.glsl shaders.
bitflags! {
    #[repr(C)]
    struct RenderingGlyphFlags: u8 {
        const COLORED   = 0b0000_0001;
        const WIDE_CHAR = 0b0000_0010;
    }
}

/// Rendering passes, for both GLES2 and GLSL3 renderer.
#[repr(u8)]
enum RenderingPass {
    /// Rendering pass used to render background color in text shaders.
    Background = 0,

    /// The first pass to render text with both GLES2 and GLSL3 renderers.
    SubpixelPass1 = 1,

    /// The second pass to render text with GLES2 renderer.
    SubpixelPass2 = 2,

    /// The third pass to render text with GLES2 renderer.
    SubpixelPass3 = 3,
}

pub trait TextRenderer<'a> {
    type Shader: TextShader;
    type RenderBatch: TextRenderBatch;
    type RenderApi: TextRenderApi<Self::RenderBatch>;

    /// Get loader API for the renderer.
    fn loader_api(&mut self) -> LoaderApi<'_>;

    /// Draw cells.
    fn draw_cells<'b: 'a, I: Iterator<Item = RenderableCell>>(
Repository: alacritty/alacritty
File: alacritty/src/renderer/mod.rs (Rust)

Code before:
        static EXTENSIONS_LOADED: Once = Once::new();

        EXTENSIONS_LOADED.call_once(|| unsafe {
            let extensions = gl::GetString(gl::EXTENSIONS);

            if extensions.is_null() {
                let mut extensions_number = 0;
                gl::GetIntegerv(gl::NUM_EXTENSIONS, &mut extensions_number);

                OPENGL_EXTENSIONS = Some(
                    (0..extensions_number as gl::types::GLuint)
                        .flat_map(|i| {
                            let extension =
                                CStr::from_ptr(gl::GetStringi(gl::EXTENSIONS, i) as *mut _);
                            extension.to_str()
                        })
                        .collect(),
                );

                return;
            }

            OPENGL_EXTENSIONS = match CStr::from_ptr(extensions as *mut _).to_str() {
                Ok(ext) => Some(ext.split_whitespace().collect()),
                Err(_) => Default::default(),
            };
        });

        unsafe {
            OPENGL_EXTENSIONS
                .as_mut()
                .map(|extensions| extensions.contains(extension))
                .unwrap_or_default()
        }
    }
}


Review comment (suggestion):
I think I'd prefer an `else` over an early return here.

Code after:

        OPENGL_EXTENSIONS.get_or_init(Self::load_extensions).contains(extension)
    }

    /// Load available OpenGL extensions.
    fn load_extensions() -> HashSet<&'static str> {
        unsafe {
            let extensions = gl::GetString(gl::EXTENSIONS);

            if extensions.is_null() {
                let mut extensions_number = 0;
                gl::GetIntegerv(gl::NUM_EXTENSIONS, &mut extensions_number);

                (0..extensions_number as gl::types::GLuint)
                    .flat_map(|i| {
                        let extension = CStr::from_ptr(gl::GetStringi(gl::EXTENSIONS, i) as *mut _);
                        extension.to_str()
                    })
                    .collect()
            } else {
                match CStr::from_ptr(extensions as *mut _).to_str() {
                    Ok(ext) => ext.split_whitespace().collect(),
                    Err(_) => HashSet::new(),
                }
            }
        }
    }
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/event.rs (Rust)

Code before:
    /// Notify that an escape sequence should be written to the PTY.
    ///
    /// TODO this needs to be able to error somehow.
    fn notify<B: Into<Cow<'static, [u8]>>>(&self, _: B);
}

#[derive(Copy, Clone, Debug)]
pub struct WindowSize {
    pub num_lines: u16,
    pub num_cols: u16,
    pub cell_width: u16,
    pub cell_height: u16,
}

/// Types that are interested in when the display is resized.
pub trait OnResize {
    fn on_resize(&mut self, window_size: WindowSize);
}

/// Event Loop for notifying the renderer about terminal events.
pub trait EventListener {
    fn send_event(&self, _event: Event) {}
}

/// It listens to the void and tests.
pub struct VoidListener;

impl EventListener for VoidListener {}


Review comment (suggestion):
Having it as `cfg(test)` prevents `alacritty` crate for using it for tests. So I've added a dummy type pure for tests, so we can use `mock_term`. Though, I can move it to `alacritty_terminal::term::tests` so it'll be more clear what it used for.

Code after:
    /// Notify that an escape sequence should be written to the PTY.
    ///
    /// TODO this needs to be able to error somehow.
    fn notify<B: Into<Cow<'static, [u8]>>>(&self, _: B);
}

#[derive(Copy, Clone, Debug)]
pub struct WindowSize {
    pub num_lines: u16,
    pub num_cols: u16,
    pub cell_width: u16,
    pub cell_height: u16,
}

/// Types that are interested in when the display is resized.
pub trait OnResize {
    fn on_resize(&mut self, window_size: WindowSize);
}

/// Event Loop for notifying the renderer about terminal events.
pub trait EventListener {
    fn send_event(&self, _event: Event) {}
}

/// Null sink for events.
pub struct VoidListener;

impl EventListener for VoidListener {}
Repository: alacritty/alacritty
File: alacritty/src/display/mod.rs (Rust)

Code before:

    /// 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,

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

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

/// The state of the input method on the `Display`.
#[derive(Debug, Default)]
pub struct Ime {
    /// Whether the IME is enabled on the given window.
    enabled: bool,

    /// Current IME preedit.
    preedit: Option<Preedit>,
}

impl Ime {
    pub fn new() -> Self {
        Default::default()
    }

    #[inline]
    pub fn set_enabled(&mut self, is_enabled: bool) {
        if is_enabled {
            self.enabled = is_enabled
        } else {
            // Clear state when disabling IME.
            *self = Default::default();
        }
    }

    #[inline]

Review comment (suggestion):
```suggestion
/// Input method state.
```

We shouldn't state where a struct is stored, since that'll just cause trouble when its location changes in the future.

Code after:

    /// 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,

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

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

/// Input method state.
#[derive(Debug, Default)]
pub struct Ime {
    /// Whether the IME is enabled.
    enabled: bool,

    /// Current IME preedit.
    preedit: Option<Preedit>,
}

impl Ime {
    pub fn new() -> Self {
        Default::default()
    }

    #[inline]
    pub fn set_enabled(&mut self, is_enabled: bool) {
        if is_enabled {
            self.enabled = is_enabled
        } else {
            // Clear state when disabling IME.
            *self = Default::default();
        }
    }

    #[inline]
Repository: alacritty/alacritty
File: alacritty/src/display/mod.rs (Rust)

Code before:

    /// 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,

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

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

/// The state of the input method on the `Display`.
#[derive(Debug, Default)]
pub struct Ime {
    /// Whether the IME is enabled on the given window.
    enabled: bool,

    /// Current IME preedit.
    preedit: Option<Preedit>,
}

impl Ime {
    pub fn new() -> Self {
        Default::default()
    }

    #[inline]
    pub fn set_enabled(&mut self, is_enabled: bool) {
        if is_enabled {
            self.enabled = is_enabled
        } else {
            // Clear state when disabling IME.
            *self = Default::default();
        }
    }

    #[inline]
    pub fn is_enabled(&self) -> bool {
        self.enabled
    }

Review comment (suggestion):
This here and the wording of "The ime on the given display" in the `Display` struct are both kinda confusing. You're talking about "the given window" and "the given display" here, but these are part of the display, there's nothing given to them. We're not talking about a function here where you give parameters when calling it and it's not a generic parameter either. So the meaning is somewhat unclear.

I think it would be fine just removing that part completely, considering it should be irrelevant in these cases, but an alternative might be just mentioning the association without using the "on the given …" wording.

Code after:

    /// 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,

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

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

/// Input method state.
#[derive(Debug, Default)]
pub struct Ime {
    /// Whether the IME is enabled.
    enabled: bool,

    /// Current IME preedit.
    preedit: Option<Preedit>,
}

impl Ime {
    pub fn new() -> Self {
        Default::default()
    }

    #[inline]
    pub fn set_enabled(&mut self, is_enabled: bool) {
        if is_enabled {
            self.enabled = is_enabled
        } else {
            // Clear state when disabling IME.
            *self = Default::default();
        }
    }

    #[inline]
    pub fn is_enabled(&self) -> bool {
        self.enabled
    }
Repository: alacritty/alacritty
File: alacritty/src/config/debug.rs (Rust)

Code before:
use log::LevelFilter;

use serde::Deserialize;

use alacritty_config_derive::ConfigDeserialize;

/// Debugging options.
#[derive(ConfigDeserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub struct Debug {
    pub log_level: LevelFilter,

    pub print_events: bool,

    /// Keep the log file after quitting.
    pub persistent_logging: bool,

    /// Should show render timer.
    pub render_timer: bool,

    /// Highlight damage information produced by alacritty.
    pub highlight_damage: bool,

    /// The renderer alacritty should be using
    pub renderer: Option<RendererPreference>,

    /// Record ref test.
    #[config(skip)]
    pub ref_test: bool,
}

impl Default for Debug {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Warn,
            print_events: Default::default(),
            persistent_logging: Default::default(),
            render_timer: Default::default(),
            highlight_damage: Default::default(),
            ref_test: Default::default(),
            renderer: Default::default(),
        }
    }
}

/// The renderer configuration options.
#[derive(Deserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum RendererPreference {
    /// OpenGL 3.3 renderer.

Review comment (suggestion):
```suggestion
    /// The renderer alacritty should be using.
```

Code after:
use log::LevelFilter;

use serde::Deserialize;

use alacritty_config_derive::ConfigDeserialize;

/// Debugging options.
#[derive(ConfigDeserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub struct Debug {
    pub log_level: LevelFilter,

    pub print_events: bool,

    /// Keep the log file after quitting.
    pub persistent_logging: bool,

    /// Should show render timer.
    pub render_timer: bool,

    /// Highlight damage information produced by alacritty.
    pub highlight_damage: bool,

    /// The renderer alacritty should be using.
    pub renderer: Option<RendererPreference>,

    /// Record ref test.
    #[config(skip)]
    pub ref_test: bool,
}

impl Default for Debug {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Warn,
            print_events: Default::default(),
            persistent_logging: Default::default(),
            render_timer: Default::default(),
            highlight_damage: Default::default(),
            ref_test: Default::default(),
            renderer: Default::default(),
        }
    }
}

/// The renderer configuration options.
#[derive(Deserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum RendererPreference {
    /// OpenGL 3.3 renderer.
Repository: alacritty/alacritty
File: alacritty/src/config/debug.rs (Rust)

Code before:
}

impl Default for Debug {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Warn,
            print_events: Default::default(),
            persistent_logging: Default::default(),
            render_timer: Default::default(),
            highlight_damage: Default::default(),
            ref_test: Default::default(),
            renderer: Default::default(),
        }
    }
}

/// The renderer configuration options.
#[derive(Deserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum RendererPreference {
    /// OpenGL 3.3 renderer.
    #[serde(rename = "glsl3")]
    Glsl3,

    /// GLES 2 renderer, but can use extensions like dual source blending.
    #[serde(rename = "gles2")]
    Gles2,

    /// Pure GLES 2 renderer.
    #[serde(rename = "gles2_pure")]
    Gles2Pure,
}


Review comment (suggestion):
```suggestion
    /// GLES 2 renderer, with optional extensions like dual source blending.
    #[serde(rename = "gles2")]
    Gles2,
```

Code after:
}

impl Default for Debug {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Warn,
            print_events: Default::default(),
            persistent_logging: Default::default(),
            render_timer: Default::default(),
            highlight_damage: Default::default(),
            ref_test: Default::default(),
            renderer: Default::default(),
        }
    }
}

/// The renderer configuration options.
#[derive(Deserialize, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum RendererPreference {
    /// OpenGL 3.3 renderer.
    #[serde(rename = "glsl3")]
    Glsl3,

    /// GLES 2 renderer, with optional extensions like dual source blending.
    #[serde(rename = "gles2")]
    Gles2,

    /// Pure GLES 2 renderer.
    #[serde(rename = "gles2_pure")]
    Gles2Pure,
}
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 based 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
    /// The state of the timer for frame scheduling.
    pub frame_timer_info: FrameTimerInfo,
```

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;