ronantakizawa/codereview-bench huggingface dataset:
Repository: google/OpenSK
File: libraries/persistent_store/src/bitfield.rs (Rust)

Code before:
///
/// A bit field is a contiguous sequence of bits in a 32-bits word.
///
/// # Invariant
///
/// - `pos + len < 32`.
pub struct Field {
    /// The position of the bit field.
    pub pos: usize,

    /// The length of the bit field.
    pub len: usize,
}

impl Field {
    /// Reads the value of a bit field.
    pub fn get(&self, word: u32) -> usize {
        ((word >> self.pos) & self.mask()) as usize
    }

    /// Sets the value of a bit field.
    ///
    /// # Preconditions
    ///
    /// - `num_bits(value) < self.len`.
    /// - `self.get(*word) & value == value`.
    pub fn set(&self, word: &mut u32, value: usize) {
        let value = value as u32;
        debug_assert_eq!(value & self.mask(), value);
        let mask = !(self.mask() << self.pos);
        *word &= mask | (value << self.pos);
        debug_assert_eq!(self.get(*word), value as usize);
    }

    /// Returns a bit mask the length of the bit field.
    fn mask(&self) -> u32 {
        (1 << self.len) - 1
    }
}

/// Represents a constant bit field.
///
/// # Invariant
///
/// - `num_bits(value) <= field.len`.
pub struct Const {
    /// The bit field.
    pub field: Field,

    /// The constant value.
    pub value: usize,

Review comment (bug):
Only one of the preconditions is `debug_assert`ed?

Code after:
///
/// A bit field is a contiguous sequence of bits in a 32-bits word.
///
/// # Invariant
///
/// - The bit field must fit in a 32-bits word: `pos + len < 32`.
pub struct Field {
    /// The position of the bit field.
    pub pos: usize,

    /// The length of the bit field.
    pub len: usize,
}

impl Field {
    /// Reads the value of a bit field.
    pub fn get(&self, word: u32) -> usize {
        ((word >> self.pos) & self.mask()) as usize
    }

    /// Sets the value of a bit field.
    ///
    /// # Preconditions
    ///
    /// - The value must fit in the bit field: `num_bits(value) < self.len`.
    /// - The value must only change bits from 1 to 0: `self.get(*word) & value == value`.
    pub fn set(&self, word: &mut u32, value: usize) {
        let value = value as u32;
        debug_assert_eq!(value & self.mask(), value);
        let mask = !(self.mask() << self.pos);
        *word &= mask | (value << self.pos);
        debug_assert_eq!(self.get(*word), value as usize);
    }

    /// Returns a bit mask the length of the bit field.
    ///
    /// The mask is meant to be applied on a value. It should be shifted to be applied to the bit
    /// field.
    fn mask(&self) -> u32 {
        (1 << self.len) - 1
    }
}

/// Represents a constant bit field.
///
/// # Invariant
///
/// - The value must fit in the bit field: `num_bits(value) <= field.len`.
pub struct ConstField {
    /// The bit field.
    pub field: Field,
Repository: google/OpenSK
File: libraries/persistent_store/src/bitfield.rs (Rust)

Code before:
    /// Sets the value of a bit field.
    ///
    /// # Preconditions
    ///
    /// - `num_bits(value) < self.len`.
    /// - `self.get(*word) & value == value`.
    pub fn set(&self, word: &mut u32, value: usize) {
        let value = value as u32;
        debug_assert_eq!(value & self.mask(), value);
        let mask = !(self.mask() << self.pos);
        *word &= mask | (value << self.pos);
        debug_assert_eq!(self.get(*word), value as usize);
    }

    /// Returns a bit mask the length of the bit field.
    fn mask(&self) -> u32 {
        (1 << self.len) - 1
    }
}

/// Represents a constant bit field.
///
/// # Invariant
///
/// - `num_bits(value) <= field.len`.
pub struct Const {
    /// The bit field.
    pub field: Field,

    /// The constant value.
    pub value: usize,
}

impl Const {
    /// Checks that the bit field has its value.
    pub fn check(&self, word: u32) -> bool {
        self.field.get(word) == self.value
    }

    /// Sets the bit field to its value.
    pub fn set(&self, word: &mut u32) {
        self.field.set(word, self.value);
    }
}

/// Represents a single bit.
///
/// # Invariant
///
/// - `pos < 32`.
pub struct Bit {

Review comment (suggestion):
`Const` is a keyword in so many languages, I think this name might confuse readers? Also doesn't really contain that it is a field at its heart. I imagine something like `BitPattern`, but there is probably something better.

Code after:
    /// Sets the value of a bit field.
    ///
    /// # Preconditions
    ///
    /// - The value must fit in the bit field: `num_bits(value) < self.len`.
    /// - The value must only change bits from 1 to 0: `self.get(*word) & value == value`.
    pub fn set(&self, word: &mut u32, value: usize) {
        let value = value as u32;
        debug_assert_eq!(value & self.mask(), value);
        let mask = !(self.mask() << self.pos);
        *word &= mask | (value << self.pos);
        debug_assert_eq!(self.get(*word), value as usize);
    }

    /// Returns a bit mask the length of the bit field.
    ///
    /// The mask is meant to be applied on a value. It should be shifted to be applied to the bit
    /// field.
    fn mask(&self) -> u32 {
        (1 << self.len) - 1
    }
}

/// Represents a constant bit field.
///
/// # Invariant
///
/// - The value must fit in the bit field: `num_bits(value) <= field.len`.
pub struct ConstField {
    /// The bit field.
    pub field: Field,

    /// The constant value.
    pub value: usize,
}

impl ConstField {
    /// Checks that the bit field has its value.
    pub fn check(&self, word: u32) -> bool {
        self.field.get(word) == self.value
    }

    /// Sets the bit field to its value.
    pub fn set(&self, word: &mut u32) {
        self.field.set(word, self.value);
    }
}

/// Represents a single bit.
///
/// # Invariant
Repository: google/OpenSK
File: third_party/libtock-drivers/src/nfc.rs (Rust)

Code before:
}

mod allow_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
}

pub struct NfcTag {}

impl NfcTag {
    pub fn enable_emulation() {
        NfcTag::emulate(true);
    }

    pub fn disable_emulation() {
        NfcTag::emulate(false);
    }

    pub fn emulate(enabled: bool) -> bool {
        let result_code =
            syscalls::command(DRIVER_NUMBER, command_nr::EMULATE, enabled as usize, 0);
        if result_code.is_err() {
            return false;
        }

        true
    }

    /// Subscribe to the tag being SELECTED callback.
    pub fn selected() -> bool {
        let is_selected = Cell::new(false);
        let mut is_selected_alarm = || is_selected.set(true);
        let subscription = syscalls::subscribe::<callback::Identity0Consumer, _>(
            DRIVER_NUMBER,
            subscribe_nr::SELECT,
            &mut is_selected_alarm,
        );
        if subscription.is_err() {
            return false;
        }

        util::yieldk_for(|| is_selected.get());
        true
    }

    /// Configure the tag type command.
    pub fn configure(tag_type: u8) -> bool {
        let result_code =
            syscalls::command(DRIVER_NUMBER, command_nr::CONFIGURE, tag_type as usize, 0);
        if result_code.is_err() {
            return false;

Review comment (suggestion):
You can just return `!result_code.is_err()`, or even better `result_code.is_ok()`. And you probably don't need the variable, if you don't want it for documentation purposes, then `syscalls::command(...).is_ok()` as a 1-line function works.

Code after:
    pub const RECEIVE: usize = 2;
}

mod allow_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
}

#[allow(dead_code)]
#[derive(Clone, Copy)]
pub struct RecvOp {
    pub result_code: usize,
    pub recv_amount: usize,
}

pub struct NfcTag {}

impl NfcTag {
    /// Check the existence of an NFC driver.
    pub fn setup() -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::CHECK, 0, 0).is_ok()
    }

    pub fn enable_emulation() -> bool {
        NfcTag::emulate(true)
    }

    pub fn disable_emulation() -> bool {
        NfcTag::emulate(false)
    }

    fn emulate(enabled: bool) -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::EMULATE, enabled as usize, 0).is_ok()
    }

    /// Configure the tag type command.
    pub fn configure(tag_type: u8) -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::CONFIGURE, tag_type as usize, 0).is_ok()
    }

    /// 1. Share with the driver a buffer.
    /// 2. Subscribe to having a successful receive callback.
    /// 3. Issue the request for reception.
    pub fn receive(buf: &mut [u8; 256]) -> TockResult<RecvOp> {
        let result = syscalls::allow(DRIVER_NUMBER, allow_nr::RECEIVE, buf)?;
        // set callback with 2 arguments, to receive ReturnCode and RX Amount
        let recv_data = Cell::new(None);
        let mut callback = |result, amount| {
            recv_data.set(Some(RecvOp {
                result_code: result,
                recv_amount: amount,
Repository: google/OpenSK
File: deploy.py (Python)

Code before:
      const="crypto_bench",
      help=("Compiles and installs the crypto_bench example that benchmarks "
            "the performance of the cryptographic algorithms on the board."))
  apps_group.add_argument(
      "--panic_test",
      dest="application",
      action="store_const",
      const="panic_test",
      help=("Compiles and installs the panic_test example that immediately "
            "triggers a panic."))
  apps_group.add_argument(
      "--oom_test",
      dest="application",
      action="store_const",
      const="oom_test",
      help=("Compiles and installs the oom_test example that tests the "
            "allocator until an out-of-memory error occurs."))
  apps_group.add_argument(
      "--console_test",
      dest="application",
      action="store_const",
      const="console_test",
      help=("Compiles and installs the console_test example that tests the "
            "console driver with messages of various lengths."))
  apps_group.add_argument(
      "--nfct_test",
      dest="application",
      action="store_const",
      const="nfct_test",
      help=("Compiles and installs the nfct_test example that tests the "
            "NFC driver."))

  main_parser.set_defaults(features=["with_ctap1"])

  main(main_parser.parse_args())


Review comment (suggestion):
It seems that the `examples/nfct_test.rs` file was added and then deleted as part of this PR.
Therefore running `deploy.py --nfct_test` will result in an error.

Code after:
      action="store_const",
      const="ctap2",
      help="Compiles and installs the OpenSK application.")
  apps_group.add_argument(
      "--crypto_bench",
      dest="application",
      action="store_const",
      const="crypto_bench",
      help=("Compiles and installs the crypto_bench example that benchmarks "
            "the performance of the cryptographic algorithms on the board."))
  apps_group.add_argument(
      "--panic_test",
      dest="application",
      action="store_const",
      const="panic_test",
      help=("Compiles and installs the panic_test example that immediately "
            "triggers a panic."))
  apps_group.add_argument(
      "--oom_test",
      dest="application",
      action="store_const",
      const="oom_test",
      help=("Compiles and installs the oom_test example that tests the "
            "allocator until an out-of-memory error occurs."))
  apps_group.add_argument(
      "--console_test",
      dest="application",
      action="store_const",
      const="console_test",
      help=("Compiles and installs the console_test example that tests the "
            "console driver with messages of various lengths."))

  main_parser.set_defaults(features=["with_ctap1"])

  main(main_parser.parse_args())
Repository: google/OpenSK
File: third_party/libtock-drivers/src/nfc.rs (Rust)

Code before:

mod command_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
    pub const EMULATE: usize = 3;
    pub const CONFIGURE: usize = 4;
    pub const FRAMEDELAYMAX: usize = 5;
}

mod subscribe_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
    pub const SELECT: usize = 3;
}

mod allow_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
}

pub enum TransOrRecvStatus {
    Error,
    InvalidBuffer,
    OOM,
    Success,
}

pub struct NfcTag {}

impl NfcTag {
    pub fn enable_emulation() -> bool {
        NfcTag::emulate(true)
    }

    pub fn disable_emulation() -> bool {
        NfcTag::emulate(false)
    }

    fn emulate(enabled: bool) -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::EMULATE, enabled as usize, 0).is_ok()
    }

    /// Subscribe to the tag being SELECTED callback.
    pub fn selected() -> bool {
        let is_selected = Cell::new(false);
        let mut is_selected_alarm = || is_selected.set(true);
        let subscription = syscalls::subscribe::<callback::Identity0Consumer, _>(
            DRIVER_NUMBER,
            subscribe_nr::SELECT,
            &mut is_selected_alarm,
        );

Review comment (suggestion):
As suggested below, you could either reuse an existing error code `enum` or remove `Success` and use it inside of `Result`s.

Code after:
const DRIVER_NUMBER: usize = 0x30003;

mod command_nr {
    pub const CHECK: usize = 0;
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
    pub const EMULATE: usize = 3;
    pub const CONFIGURE: usize = 4;
}

mod subscribe_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
}

mod allow_nr {
    pub const TRANSMIT: usize = 1;
    pub const RECEIVE: usize = 2;
}

#[allow(dead_code)]
#[derive(Clone, Copy)]
pub struct RecvOp {
    pub result_code: usize,
    pub recv_amount: usize,
}

pub struct NfcTag {}

impl NfcTag {
    /// Check the existence of an NFC driver.
    pub fn setup() -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::CHECK, 0, 0).is_ok()
    }

    pub fn enable_emulation() -> bool {
        NfcTag::emulate(true)
    }

    pub fn disable_emulation() -> bool {
        NfcTag::emulate(false)
    }

    fn emulate(enabled: bool) -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::EMULATE, enabled as usize, 0).is_ok()
    }

    /// Configure the tag type command.
    pub fn configure(tag_type: u8) -> bool {
        syscalls::command(DRIVER_NUMBER, command_nr::CONFIGURE, tag_type as usize, 0).is_ok()
    }
Repository: google/OpenSK
File: src/ctap/apdu.rs (Rust)

Code before:
    }

    #[test]
    fn test_invalid_apdu_header_length() {
        let frame: [u8; 3] = [0x00, 0x12, 0x00];
        let response = pass_frame(&frame);
        assert!(response.is_err());
        assert_eq!(Some(ApduStatusCode::SW_WRONG_DATA), response.err());
    }

    #[test]
    fn test_unsupported_case_type() {
        let frame: [u8; 73] = [
            0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x40, 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d,
            0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3,
            0x56, 0x19, 0xdf, 0x4a, 0x00, 0x02, 0xae, 0x2a, 0x8c, 0x9d, 0x5a, 0xab, 0xc3, 0x4b,
            0x4e, 0xb9, 0x78, 0xb9, 0x11, 0xe5, 0x52, 0x40, 0xf3, 0x45, 0x64, 0x9c, 0xd3, 0xd7,
            0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d,
            0xcb, 0x00, 0x00,
        ];
        let response = pass_frame(&frame);
        assert!(response.is_err());
        assert_eq!(
            Some(ApduStatusCode::SW_COND_USE_NOT_SATISFIED),
            response.err()
        );
    }
}


Review comment (suggestion):
You don't need the double `assert` here and in all other tests. Just do
`assert_eq!(response, Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED));

Code after:
    fn test_case_type_4_short_le() {
        let frame: [u8; 13] = [
            0x00, 0xa4, 0x04, 0x00, 0x07, 0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01, 0x00,
        ];
        let payload = [0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01];
        let response = pass_frame(&frame);
        let expected = APDU {
            header: ApduHeader {
                cla: 0x00,
                ins: 0xa4,
                p1: 0x04,
                p2: 0x00,
            },
            lc: 0x07,
            data: payload.to_vec(),
            le: 0x100,
            case_type: ApduType::Short(Case::LcDataLe),
        };
        assert_eq!(Ok(expected), response);
    }

    #[test]
    fn test_invalid_apdu_header_length() {
        let frame: [u8; 3] = [0x00, 0x12, 0x00];
        let response = pass_frame(&frame);
        assert_eq!(Err(ApduStatusCode::SW_WRONG_DATA), response);
    }

    #[test]
    fn test_unsupported_case_type() {
        let frame: [u8; 73] = [
            0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x40, 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d,
            0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3,
            0x56, 0x19, 0xdf, 0x4a, 0x00, 0x02, 0xae, 0x2a, 0x8c, 0x9d, 0x5a, 0xab, 0xc3, 0x4b,
            0x4e, 0xb9, 0x78, 0xb9, 0x11, 0xe5, 0x52, 0x40, 0xf3, 0x45, 0x64, 0x9c, 0xd3, 0xd7,
            0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d,
            0xcb, 0x00, 0x00,
        ];
        let response = pass_frame(&frame);
        assert_eq!(Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), response);
    }
}
Repository: google/OpenSK
File: src/ctap/apdu.rs (Rust)

Code before:
            ApduStatusCode::SW_CLA_INVALID => b"\x6e\x00",
            ApduStatusCode::SW_INTERNAL_EXCEPTION => b"\x6f\x00",
        }
    }
}

#[allow(non_camel_case_types, dead_code)]
pub enum ApduIns {
    SELECT = 0xA4,
    READ_BINARY = 0xB0,
    GET_RESPONSE = 0xC0,
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(Default, PartialEq)]
pub struct ApduHeader {
    cla: u8,
    ins: u8,
    p1: u8,
    p2: u8,
}

impl From<&[u8]> for ApduHeader {
    fn from(header: &[u8]) -> Self {
        ApduHeader {
            cla: header[0],
            ins: header[1],
            p1: header[2],
            p2: header[3],
        }
    }
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(Default, PartialEq)]
pub struct APDU {
    header: ApduHeader,
    lc: u16,
    data: Vec<u8>,
    le: u32,
    case_type: u8,
}

const APDU_HEADER_LEN: u8 = 4;

impl TryFrom<&[u8]> for APDU {
    type Error = ApduStatusCode;

    fn try_from(frame: &[u8]) -> Result<Self, ApduStatusCode> {

Review comment (suggestion):
We'd probably want to construct it from an array (or array reference) of length 4 to avoid out of bounds problems. You declare the length as a constant to work around this problem. Better to enforce it here.

Code after:
            ApduStatusCode::SW_INCORRECT_P1P2 => b"\x6a\x86",
            ApduStatusCode::SW_INS_INVALID => b"\x6d\x00",
            ApduStatusCode::SW_CLA_INVALID => b"\x6e\x00",
            ApduStatusCode::SW_INTERNAL_EXCEPTION => b"\x6f\x00",
        }
    }
}

#[allow(dead_code)]
pub enum ApduInstructions {
    Select = 0xA4,
    ReadBinary = 0xB0,
    GetResponse = 0xC0,
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(Default, PartialEq)]
pub struct ApduHeader {
    cla: u8,
    ins: u8,
    p1: u8,
    p2: u8,
}

impl From<&[u8]> for ApduHeader {
    fn from(header: &[u8]) -> Self {
        ApduHeader {
            cla: header[0],
            ins: header[1],
            p1: header[2],
            p2: header[3],
        }
    }
}

#[cfg_attr(test, derive(Clone, Debug))]
#[derive(PartialEq)]
/// The APDU cases
pub enum Case {
    Le,
    LcData,
    LcDataLe,
    // TODO: More cases to add as extended length APDUs
    // Le could be 2 or 3 Bytes
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(PartialEq)]
pub enum ApduType {
Repository: google/OpenSK
File: src/ctap/apdu.rs (Rust)

Code before:
    cla: u8,
    ins: u8,
    p1: u8,
    p2: u8,
}

impl From<&[u8]> for ApduHeader {
    fn from(header: &[u8]) -> Self {
        ApduHeader {
            cla: header[0],
            ins: header[1],
            p1: header[2],
            p2: header[3],
        }
    }
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(Default, PartialEq)]
pub struct APDU {
    header: ApduHeader,
    lc: u16,
    data: Vec<u8>,
    le: u32,
    case_type: u8,
}

const APDU_HEADER_LEN: u8 = 4;

impl TryFrom<&[u8]> for APDU {
    type Error = ApduStatusCode;

    fn try_from(frame: &[u8]) -> Result<Self, ApduStatusCode> {
        if frame.len() < APDU_HEADER_LEN as usize {
            return Err(ApduStatusCode::SW_WRONG_DATA);
        }
        //        +-----+-----+----+----+
        // header | CLA | INS | P1 | P2 |
        //        +-----+-----+----+----+
        let (header, payload) = frame.split_at(APDU_HEADER_LEN as usize);

        let mut apdu = APDU {
            header: header.into(),
            lc: 0x00,
            data: Vec::new(),
            le: 0x00,
            case_type: 0x00,
        };

        // case 1

Review comment (suggestion):
This is not part of an APDU. Can you describe what's the purpose of `case_type`?
Also by the name of it, it somehow screams to be an `enum` ;-)

Code after:
#[derive(Default, PartialEq)]
pub struct ApduHeader {
    cla: u8,
    ins: u8,
    p1: u8,
    p2: u8,
}

impl From<&[u8]> for ApduHeader {
    fn from(header: &[u8]) -> Self {
        ApduHeader {
            cla: header[0],
            ins: header[1],
            p1: header[2],
            p2: header[3],
        }
    }
}

#[cfg_attr(test, derive(Clone, Debug))]
#[derive(PartialEq)]
/// The APDU cases
pub enum Case {
    Le,
    LcData,
    LcDataLe,
    // TODO: More cases to add as extended length APDUs
    // Le could be 2 or 3 Bytes
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(PartialEq)]
pub enum ApduType {
    Instruction,
    Short(Case),
    Extended(Case),
    Unknown,
}

impl Default for ApduType {
    fn default() -> ApduType {
        ApduType::Unknown
    }
}

#[cfg_attr(test, derive(Clone, Debug))]
#[allow(dead_code)]
#[derive(Default, PartialEq)]
pub struct APDU {
    header: ApduHeader,