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

Code before:
        const CONTROL = 0b0000_0100;
        const SUPER   = 0b0000_1000;
        // NOTE: Kitty protocol defines additional modifiers to what is present here, like
        // Capslock, but it's not a modifier as per winit.
    }
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check that the text represents either C0 or C1 codepoint.
fn is_c0c1_codepoint(text: &str) -> bool {
    let codepoint = text.chars().next().map(u32::from).unwrap_or(0);
    // Include 0x7f as control character.
    text.len() <= 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}


Review comment (suggestion):
```suggestion
/// Check whether the text represents a C0 or C1 codepoint.
```

Code after:
        const ALT     = 0b0000_0010;
        const CONTROL = 0b0000_0100;
        const SUPER   = 0b0000_1000;
        // NOTE: Kitty protocol defines additional modifiers to what is present here, like
        // Capslock, but it's not a modifier as per winit.
    }
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check whether the `text` is `0x7f`, `C0` or `C1` control code.
fn is_control_character(text: &str) -> bool {
    // 0x7f (DEL) is included here since it has a dedicated control code (`^?`) which generally
    // does not match the reported text (`^H`), despite not technically being part of C0 or C1.
    let codepoint = text.bytes().next().unwrap();
    text.len() == 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}
Repository: alacritty/alacritty
File: alacritty/src/input/keyboard.rs (Rust)

Code before:
        // Capslock, but it's not a modifier as per winit.
    }
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check that the text represents either C0 or C1 codepoint.
fn is_c0c1_codepoint(text: &str) -> bool {
    let codepoint = text.chars().next().map(u32::from).unwrap_or(0);
    // Include 0x7f as control character.
    text.len() <= 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}


Review comment (suggestion):
This comment strikes me as odd because it doesn't document what the code does nor does it explain why `0x7f` is special and included.

Suggestion:

```
// 0x7f (DEL) is included here since it has a dedicated control code (`^?`) which generally does not match the reported text (`^H`), despite not technically being part of C0 or C1.
```

Not sure if this explanation matches your reasoning, but I'd assume it's something like that.

Code after:
        // NOTE: Kitty protocol defines additional modifiers to what is present here, like
        // Capslock, but it's not a modifier as per winit.
    }
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check whether the `text` is `0x7f`, `C0` or `C1` control code.
fn is_control_character(text: &str) -> bool {
    // 0x7f (DEL) is included here since it has a dedicated control code (`^?`) which generally
    // does not match the reported text (`^H`), despite not technically being part of C0 or C1.
    let codepoint = text.bytes().next().unwrap();
    text.len() == 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}
Repository: alacritty/alacritty
File: alacritty/src/input/keyboard.rs (Rust)

Code before:
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check whether the text represents a C0 or C1 codepoint.
fn is_c0c1_codepoint(text: &str) -> bool {
    let codepoint = text.bytes().next().unwrap();
    // 0x7f (DEL) is included here since it has a dedicated control code (`^?`) which generally
    // does not match the reported text (`^H`), despite not technically being part of C0 or C1.
    text.bytes().len() == 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}


Review comment (suggestion):
Is there any difference between `text.bytes().len()` and `text.len()`? Isn't `text.len()` the byte length anyway?

And even if it was the character length instead, it would still work since the only characters that start with the control codes are the C0 and C1s.

Code after:
}

impl SequenceModifiers {
    /// Get the value which should be passed to escape sequence.
    pub fn encode_esc_sequence(self) -> u8 {
        self.bits() + 1
    }
}

impl From<ModifiersState> for SequenceModifiers {
    fn from(mods: ModifiersState) -> Self {
        let mut modifiers = Self::empty();
        modifiers.set(Self::SHIFT, mods.shift_key());
        modifiers.set(Self::ALT, mods.alt_key());
        modifiers.set(Self::CONTROL, mods.control_key());
        modifiers.set(Self::SUPER, mods.super_key());
        modifiers
    }
}

/// Check whether the `text` is `0x7f`, `C0` or `C1` control code.
fn is_control_character(text: &str) -> bool {
    // 0x7f (DEL) is included here since it has a dedicated control code (`^?`) which generally
    // does not match the reported text (`^H`), despite not technically being part of C0 or C1.
    let codepoint = text.bytes().next().unwrap();
    text.len() == 1 && (codepoint < 0x20 || (0x7f..=0x9f).contains(&codepoint))
}
Repository: alacritty/alacritty
File: alacritty_config_derive/tests/config.rs (Rust)

Code before:
fn field_replacement() {
    let mut test = Test::default();

    let value = toml::from_str("nesting.field2=13").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.nesting.field2, Some(13));
}

#[test]
fn replace_derive() {
    let mut test = Test::default();

    let value = toml::from_str("nesting.newtype=9").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.nesting.newtype, NewType(9));
}

#[test]
fn replace_derive_using_alias() {
    let mut test = Test::default();

    assert_ne!(test.field1, 9);

    let value = toml::from_str("noalias=9").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.field1, 9);
}

#[test]
fn replace_derive_using_multiple_aliases() {
    let mut test = Test::default();

    for (alias, value) in [("multiple_alias1", 6), ("multiple_alias2", 7)] {
        let toml_value = toml::from_str(&format!("{}={}", alias, value)).unwrap();
        test.replace(toml_value).unwrap();

        assert_eq!(test.multiple_alias_field, value);
    }
}

#[test]
fn replace_flatten() {
    let mut test = Test::default();

    let value = toml::from_str("flatty=7").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.flatten.flatty, 7);

Review comment (suggestion):
I'm not sure why this field has the alias "noalias", but would probably best to change that. It makes no sense to me.

Code after:
fn field_replacement() {
    let mut test = Test::default();

    let value = toml::from_str("nesting.field2=13").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.nesting.field2, Some(13));
}

#[test]
fn replace_derive() {
    let mut test = Test::default();

    let value = toml::from_str("nesting.newtype=9").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.nesting.newtype, NewType(9));
}

#[test]
fn replace_derive_using_alias() {
    let mut test = Test::default();

    assert_ne!(test.field1, 9);

    let value = toml::from_str("field1_alias=9").unwrap();
    test.replace(value).unwrap();

    assert_eq!(test.field1, 9);
}

#[test]
fn replace_derive_using_multiple_aliases() {
    let mut test = Test::default();

    let toml_value = toml::from_str("multiple_alias1=6").unwrap();
    test.replace(toml_value).unwrap();

    assert_eq!(test.multiple_alias_field, 6);

    let toml_value = toml::from_str("multiple_alias1=7").unwrap();
    test.replace(toml_value).unwrap();

    assert_eq!(test.multiple_alias_field, 7);
}

#[test]
fn replace_flatten() {
    let mut test = Test::default();

    let value = toml::from_str("flatty=7").unwrap();
Repository: alacritty/alacritty
File: alacritty_config_derive/src/serde_replace.rs (Rust)

Code before:
            let aliases = field
                .attrs
                .iter()
                .filter(|attr| (*attr).path().is_ident("config"))
                .filter_map(|attr| attr.parse_args::<Attr>().ok())
                .filter(|parsed| parsed.ident.as_str() == "alias")
                .map(|parsed| {
                    let value = parsed
                        .param
                        .unwrap_or_else(|| panic!("Field \"{}\" has no alias value", ident))
                        .value();

                    if value.is_empty() {
                        panic!("Field \"{}\" has an empty alias value", ident);
                    }

                    value
                })
                .collect::<Vec<_>>();

            stream.extend(quote! {
                    #(#aliases)|* | #literal => alacritty_config::SerdeReplace::replace(&mut
            self.#ident, next_value)?,         });
        }
    }

    // Add the flattened catch-all as last match arm.
    if let Some(flattened_arm) = flattened_arm.take() {
        stream.extend(flattened_arm);
    }

    stream
}


Review comment (suggestion):
My intention was originally to just put these in a separate branch, but surprisingly this seems to work just fine. `| "xxx" => _,` is valid Rust.

Code after:
            });
        } else {
            // Extract all `#[config(alias = "...")]` attribute values.
            let aliases = field
                .attrs
                .iter()
                .filter(|attr| (*attr).path().is_ident("config"))
                .filter_map(|attr| attr.parse_args::<Attr>().ok())
                .filter(|parsed| parsed.ident.as_str() == "alias")
                .map(|parsed| {
                    let value = parsed
                        .param
                        .ok_or_else(|| format!("Field \"{}\" has no alias value", ident))?
                        .value();

                    if value.trim().is_empty() {
                        return Err(format!("Field \"{}\" has an empty alias value", ident));
                    }

                    Ok(value)
                })
                .collect::<Result<Vec<String>, String>>()
                .map_err(|msg| Error::new(ident.span(), msg))?;

            stream.extend(quote! {
                    #(#aliases)|* | #literal => alacritty_config::SerdeReplace::replace(&mut
            self.#ident, next_value)?,         });
        }
    }

    // Add the flattened catch-all as last match arm.
    if let Some(flattened_arm) = flattened_arm.take() {
        stream.extend(flattened_arm);
    }

    Ok(stream)
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/tty/windows/child.rs (Rust)

Code before:
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;

    #[test]
    pub fn event_is_emitted_when_child_exits() {
        const WAIT_TIMEOUT: Duration = Duration::from_millis(200);

        let poller = Arc::new(Poller::new().unwrap());

        let mut child = Command::new("cmd.exe").spawn().unwrap();
        let child_exit_watcher = ChildExitWatcher::new(child.as_raw_handle() as HANDLE).unwrap();
        child_exit_watcher.register(&poller, Event::readable(PTY_CHILD_EVENT_TOKEN));

        child.kill().unwrap();

        // Poll for the event or fail with timeout if nothing has been sent.
        let mut events = polling::Events::new();
        poller.wait(&mut events, Some(WAIT_TIMEOUT)).unwrap();
        assert_eq!(events.iter().next().unwrap().key, PTY_CHILD_EVENT_TOKEN);
        // Verify that at least one `ChildEvent::Exited` was received.
        assert_eq!(
            child_exit_watcher.event_rx().try_recv(),
            Ok(ChildEvent::Exited(Some(1))),
            "Killed process should report non-zero exit code"
        );
    }
}


Review comment (suggestion):
The message and assertion do not match. Should say we expect an exit code of `1` if that's what we're asserting.

I'd go so far and say this message provides no additional benefit and if we wanted to add a message the most useful thing to put there would be the exit status we actually got.

Code after:
    use std::process::Command;
    use std::sync::Arc;
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;

    #[test]
    pub fn event_is_emitted_when_child_exits() {
        const WAIT_TIMEOUT: Duration = Duration::from_millis(200);

        let poller = Arc::new(Poller::new().unwrap());

        let mut child = Command::new("cmd.exe").spawn().unwrap();
        let child_exit_watcher = ChildExitWatcher::new(child.as_raw_handle() as HANDLE).unwrap();
        child_exit_watcher.register(&poller, Event::readable(PTY_CHILD_EVENT_TOKEN));

        child.kill().unwrap();

        // Poll for the event or fail with timeout if nothing has been sent.
        let mut events = polling::Events::new();
        poller.wait(&mut events, Some(WAIT_TIMEOUT)).unwrap();
        assert_eq!(events.iter().next().unwrap().key, PTY_CHILD_EVENT_TOKEN);
        // Verify that at least one `ChildEvent::Exited` was received.
        assert_eq!(child_exit_watcher.event_rx().try_recv(), Ok(ChildEvent::Exited(Some(1))));
    }
}
Repository: alacritty/alacritty
File: alacritty_terminal/src/tty/windows/child.rs (Rust)

Code before:
    }

    pub fn event_rx(&self) -> &mpsc::Receiver<ChildEvent> {
        &self.event_rx
    }

    pub fn register(&self, poller: &Arc<Poller>, event: Event) {
        *self.interest.lock().unwrap() = Some(Interest { poller: poller.clone(), event });
    }

    pub fn deregister(&self) {
        *self.interest.lock().unwrap() = None;
    }

    pub fn fd(&self) -> isize {
        self.child_handle
    }

    pub fn pid(&self) -> u32 {
        self.pid
    }
}

impl Drop for ChildExitWatcher {
    fn drop(&mut self) {
        unsafe {
            UnregisterWait(self.wait_handle.load(Ordering::Relaxed) as HANDLE);
        }
    }
}

#[cfg(test)]
mod tests {
    use std::os::windows::io::AsRawHandle;
    use std::process::Command;
    use std::sync::Arc;
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;

    #[test]
    pub fn event_is_emitted_when_child_exits() {
        const WAIT_TIMEOUT: Duration = Duration::from_millis(200);

        let poller = Arc::new(Poller::new().unwrap());

        let mut child = Command::new("cmd.exe").spawn().unwrap();
        let child_exit_watcher = ChildExitWatcher::new(child.as_raw_handle() as HANDLE).unwrap();
        child_exit_watcher.register(&poller, Event::readable(PTY_CHILD_EVENT_TOKEN));


Review comment (suggestion):
This is generally not a `FD`? You should also document what exactly is getting returned here. 

Code after:
        }
    }

    pub fn event_rx(&self) -> &mpsc::Receiver<ChildEvent> {
        &self.event_rx
    }

    pub fn register(&self, poller: &Arc<Poller>, event: Event) {
        *self.interest.lock().unwrap() = Some(Interest { poller: poller.clone(), event });
    }

    pub fn deregister(&self) {
        *self.interest.lock().unwrap() = None;
    }

    /// Retrieve the process handle of the underlying child process.
    ///
    /// This function does **not** pass ownership of the raw handle to you,
    /// and the handle is only guaranteed to be valid while the hosted application
    /// has not yet been destroyed.
    ///
    /// If you terminate the process using this handle, the terminal will get a
    /// timeout error, and the child watcher will emit an `Exited` event.
    pub fn raw_handle(&self) -> HANDLE {
        self.child_handle
    }

    /// Retrieve the Process ID associated to the underlying child process.
    pub fn pid(&self) -> Option<NonZeroU32> {
        self.pid
    }
}

impl Drop for ChildExitWatcher {
    fn drop(&mut self) {
        unsafe {
            UnregisterWait(self.wait_handle.load(Ordering::Relaxed) as HANDLE);
        }
    }
}

#[cfg(test)]
mod tests {
    use std::os::windows::io::AsRawHandle;
    use std::process::Command;
    use std::sync::Arc;
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;
Repository: alacritty/alacritty
File: alacritty_terminal/src/tty/windows/child.rs (Rust)

Code before:
    }

    pub fn event_rx(&self) -> &mpsc::Receiver<ChildEvent> {
        &self.event_rx
    }

    pub fn register(&self, poller: &Arc<Poller>, event: Event) {
        *self.interest.lock().unwrap() = Some(Interest { poller: poller.clone(), event });
    }

    pub fn deregister(&self) {
        *self.interest.lock().unwrap() = None;
    }

    /// Retrieve the process handle of the underlying child process.
    /// This function does **not** pass ownership of the raw handle to you,
    /// and the handle is only guaranteed to be valid while the hosted application
    /// has not yet been destroyed.
    /// If you terminate the process using this handle, the terminal will get a
    /// timeout error, and the child watcher will emit an `Exited` event.
    pub fn raw_handle(&self) -> HANDLE {
        self.child_handle
    }

    /// Retrieve the Process ID associated to the underlying child process.
    pub fn pid(&self) -> u32 {
        self.pid
    }
}

impl Drop for ChildExitWatcher {
    fn drop(&mut self) {
        unsafe {
            UnregisterWait(self.wait_handle.load(Ordering::Relaxed) as HANDLE);
        }
    }
}

#[cfg(test)]
mod tests {
    use std::os::windows::io::AsRawHandle;
    use std::process::Command;
    use std::sync::Arc;
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;

    #[test]
    pub fn event_is_emitted_when_child_exits() {
        const WAIT_TIMEOUT: Duration = Duration::from_millis(200);

Review comment (suggestion):
Maybe we should use `Option<NonZeroU32>` for that? Since the value is 0 when it errors and it generally can't be _zero_. Though, I'm not strong on that.

Code after:
        }
    }

    pub fn event_rx(&self) -> &mpsc::Receiver<ChildEvent> {
        &self.event_rx
    }

    pub fn register(&self, poller: &Arc<Poller>, event: Event) {
        *self.interest.lock().unwrap() = Some(Interest { poller: poller.clone(), event });
    }

    pub fn deregister(&self) {
        *self.interest.lock().unwrap() = None;
    }

    /// Retrieve the process handle of the underlying child process.
    ///
    /// This function does **not** pass ownership of the raw handle to you,
    /// and the handle is only guaranteed to be valid while the hosted application
    /// has not yet been destroyed.
    ///
    /// If you terminate the process using this handle, the terminal will get a
    /// timeout error, and the child watcher will emit an `Exited` event.
    pub fn raw_handle(&self) -> HANDLE {
        self.child_handle
    }

    /// Retrieve the Process ID associated to the underlying child process.
    pub fn pid(&self) -> Option<NonZeroU32> {
        self.pid
    }
}

impl Drop for ChildExitWatcher {
    fn drop(&mut self) {
        unsafe {
            UnregisterWait(self.wait_handle.load(Ordering::Relaxed) as HANDLE);
        }
    }
}

#[cfg(test)]
mod tests {
    use std::os::windows::io::AsRawHandle;
    use std::process::Command;
    use std::sync::Arc;
    use std::time::Duration;

    use super::super::PTY_CHILD_EVENT_TOKEN;
    use super::*;