ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/edit
File: src/buffer/line_cache.rs (Rust)

Code before:
use std::ops::Range;

use crate::{document::ReadableDocument, simd::memchr2};

/// Cache a line/offset pair every CACHE_EVERY lines to speed up line/offset calculations
const CACHE_EVERY: usize = 1024 * 64;

pub struct LineCache {
    cache: Vec<(usize, usize)>, // (index, line)
}

impl LineCache {
    pub fn new() -> Self {
        Self { cache: vec![] }
    }

    pub fn from_document<T: ReadableDocument>(&mut self, document: &T) {
        self.cache.clear();

        let mut offset = 0;
        let mut line = 0;
        loop {
            let text = document.read_forward(offset);
            if text.is_empty() { return; }
            
            let mut off = 0;
            loop {
                off = memchr2(b'\n', b'\n', text, off);
                if off == text.len() { break; }

                if line % CACHE_EVERY == 0 {
                    self.cache.push((offset+off, line));
                }
                line += 1;

Review comment (suggestion):
Instead of storing indices/lines, ideally, we would store index/line _deltas_ between items. During updates, we then only need to update the items that are affected by the change, but not all items past it.

Code after:
use std::ops::Range;

use crate::{document::ReadableDocument, simd::memchr2};

/// Cache a line/offset pair every CACHE_EVERY lines to speed up line/offset calculations
const CACHE_EVERY: usize = 1024 * 64;

#[derive(Clone)]
pub struct CachePoint {
    pub index: usize,
    pub line: usize,
    // pub snapshot: ParserSnapshot
}

pub struct LineCache {
    cache: Vec<CachePoint>,
}

impl LineCache {
    pub fn new() -> Self {
        Self { cache: vec![] }
    }

    pub fn from_document<T: ReadableDocument>(&mut self, document: &T) {
        self.cache.clear();

        let mut offset = 0;
        let mut line = 0;
        loop {
            let text = document.read_forward(offset);
            if text.is_empty() { return; }
            
            let mut off = 0;
            loop {
Repository: microsoft/edit
File: src/buffer/line_cache.rs (Rust)

Code before:
        for &mut (ref mut off, ref mut line) in &mut self.cache {
            if *off > offset {
                *off += len;
                *line += newlines;
            }
        }
    }

    /// Finds the nearest cached line-offset pair relative to a target line.
    /// If `reverse` is false, it returns the closest *before* the target.
    /// If `reverse` is true, it returns the closest *after or at* the target.
    pub fn nearest_offset(&self, target_count: usize, reverse: bool) -> Option<(usize, usize)> {
        let len = self.cache.len();
        let mut i = 0;
        while i < len {
            if self.cache[i].1 >= target_count {
                let ind = if !reverse {
                    if i == 0 { return None; } // No previous line exists
                    i - 1
                } else {
                    if i == len - 1 { return None; } // No next line exists
                    i
                };
                return Some(self.cache[ind]);
            }
            i += 1;
        }

        None
    }
}


Review comment (performance):
I believe this should ideally be a binary search, right?
If we use deltas as I proposed above, we could note the last deletion/insertion/nearest_offset position and its absolute index and cache it in a member. Then we can start iterating from there. Modifications and searches should occur near each other, after all.

Code after:
        for c in text {
            if *c == b'\n' {
                newlines += 1;
            }
        }

        let len = text.len();
        for point in &mut self.cache {
            if point.index > offset {
                point.index += len;
                point.line += newlines;
            }
        }

        // TODO: This also needs to insert new cache points
    }

    /// Finds the nearest cached line-offset pair relative to a target line.
    /// If `reverse` is false, it returns the closest *before* the target.
    /// If `reverse` is true, it returns the closest *after or at* the target.
    pub fn nearest_offset(&self, target_count: usize, reverse: bool) -> Option<CachePoint> {
        match self.cache.binary_search_by_key(&target_count, |p| p.line) {
            Ok(i) => Some(self.cache[i].clone()),
            Err(i) => {
                if i == 0 || i == self.cache.len() { None }  // target < lowest cache point || target > highest cache point
                else {
                    Some(self.cache[ if reverse {i} else {i-1} ].clone())
                }
            }
        }
    }
}
Repository: microsoft/edit
File: src/lib.rs (Rust)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#![feature(
    allocator_api,
    breakpoint,
    cold_path,
    let_chains,
    linked_list_cursors,
    maybe_uninit_fill,
    maybe_uninit_slice,
    maybe_uninit_uninit_array_transpose
)]
#![cfg_attr(target_arch = "loongarch64", feature(stdarch_loongarch))]
#![allow(clippy::missing_transmute_annotations, clippy::new_without_default, stable_features)]

#[macro_use]
pub mod arena;

pub mod apperr;
pub mod base64;
pub mod buffer;
pub mod cell;
pub mod clipboard;
pub mod document;
pub mod framebuffer;
pub mod fuzzy;
pub mod hash;
pub mod helpers;
pub mod icu;
pub mod input;
pub mod oklab;
pub mod path;
pub mod simd;
pub mod sys;
pub mod tui;
pub mod unicode;
pub mod vt;


Review comment (suggestion):
The `stdarch_loongarch_feature_detection` and `loongarch_target_feature` feature gates have been stabilized in rust 1.89.0. However, since the current stable compiler version is 1.88.0, there gates still need to be explicitly enabled. So, it still makes sense to include them, even though this project declares `nightly` in its `rust-toolchain.toml`.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#![feature(
    allocator_api,
    breakpoint,
    cold_path,
    let_chains,
    linked_list_cursors,
    maybe_uninit_fill,
    maybe_uninit_slice,
    maybe_uninit_uninit_array_transpose
)]
#![cfg_attr(
    target_arch = "loongarch64",
    feature(stdarch_loongarch, stdarch_loongarch_feature_detection, loongarch_target_feature),
    allow(clippy::incompatible_msrv)
)]
#![allow(clippy::missing_transmute_annotations, clippy::new_without_default, stable_features)]

#[macro_use]
pub mod arena;

pub mod apperr;
pub mod base64;
pub mod buffer;
pub mod cell;
pub mod clipboard;
pub mod document;
pub mod framebuffer;
pub mod fuzzy;
pub mod hash;
pub mod helpers;
pub mod icu;
pub mod input;
pub mod oklab;
pub mod path;
pub mod simd;
pub mod sys;
Repository: microsoft/edit
File: src/simd/lines_bwd.rs (Rust)

Code before:
            if line_next <= line_stop {
                break;
            }

            end = chunk_start;
            remaining -= 32;
            line = line_next;
        }

        lines_bwd_fallback(beg, end, line, line_stop)
    }
}

#[cfg(target_arch = "aarch64")]
unsafe fn lines_bwd_neon(
    beg: *const u8,
    mut end: *const u8,
    mut line: CoordType,
    line_stop: CoordType,
) -> (*const u8, CoordType) {
    unsafe {
        use std::arch::aarch64::*;

        let lf = vdupq_n_u8(b'\n');
        let line_stop = line_stop.min(line);
        let mut remaining = end.offset_from_unsigned(beg);

        while remaining >= 64 {
            let chunk_start = end.sub(64);

            let v1 = vld1q_u8(chunk_start.add(0));
            let v2 = vld1q_u8(chunk_start.add(16));
            let v3 = vld1q_u8(chunk_start.add(32));
            let v4 = vld1q_u8(chunk_start.add(48));

            let mut sum = vdupq_n_u8(0);
            sum = vsubq_u8(sum, vceqq_u8(v1, lf));
            sum = vsubq_u8(sum, vceqq_u8(v2, lf));
            sum = vsubq_u8(sum, vceqq_u8(v3, lf));
            sum = vsubq_u8(sum, vceqq_u8(v4, lf));

            let sum = vaddvq_u8(sum);

            let line_next = line - sum as CoordType;
            if line_next <= line_stop {
                break;
            }

            end = chunk_start;
            remaining -= 64;
            line = line_next;

Review comment (suggestion):
Is there value in supporting both? AVX2 is supported by most x64 devices, so that's the only thing we support. I'd prefer if we could do the same with LSX and only support LASX. This would reduce our long-term maintenance burden. But I don't know how widespread LASX support is... 

Code after:
            if line_next <= line_stop {
                break;
            }

            end = chunk_start;
            remaining -= 32;
            line = line_next;
        }

        lines_bwd_fallback(beg, end, line, line_stop)
    }
}

#[cfg(target_arch = "loongarch64")]
unsafe fn lines_bwd_dispatch(
    beg: *const u8,
    end: *const u8,
    line: CoordType,
    line_stop: CoordType,
) -> (*const u8, CoordType) {
    use std::arch::is_loongarch_feature_detected;

    let func = if is_loongarch_feature_detected!("lasx") {
        lines_bwd_lasx
    } else if is_loongarch_feature_detected!("lsx") {
        lines_bwd_lsx
    } else {
        lines_bwd_fallback
    };
    unsafe { LINES_BWD_DISPATCH = func };
    unsafe { func(beg, end, line, line_stop) }
}

#[cfg(target_arch = "loongarch64")]
#[target_feature(enable = "lasx")]
unsafe fn lines_bwd_lasx(
    beg: *const u8,
    mut end: *const u8,
    mut line: CoordType,
    line_stop: CoordType,
) -> (*const u8, CoordType) {
    unsafe {
        use std::arch::loongarch64::*;
        use std::mem::transmute as T;

        #[inline(always)]
        unsafe fn horizontal_sum(sum: v32i8) -> u32 {
            unsafe {
                let sum = lasx_xvhaddw_h_b(sum, sum);
                let sum = lasx_xvhaddw_w_h(sum, sum);
                let sum = lasx_xvhaddw_d_w(sum, sum);
Repository: microsoft/edit
File: crates/edit/src/unicode/measurement.rs (Rust)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

use std::hint::cold_path;

use super::Utf8Chars;
use super::tables::*;
use crate::document::ReadableDocument;
use crate::helpers::{CoordType, Point};

// On one hand it's disgusting that I wrote this as a global variable, but on the
// other hand, this isn't a public library API, and it makes the code a lot cleaner,
// because we don't need to inject this once-per-process value everywhere.
static mut AMBIGUOUS_WIDTH: usize = 1;

/// Sets the width of "ambiguous" width characters as per "UAX #11: East Asian Width".
///
/// Defaults to 1.
pub fn setup_ambiguous_width(ambiguous_width: CoordType) {
    unsafe { AMBIGUOUS_WIDTH = ambiguous_width as usize };
}

#[inline]
fn ambiguous_width() -> usize {
    // SAFETY: This is a global variable that is set once per process.
    // It is never changed after that, so this is safe to call.
    unsafe { AMBIGUOUS_WIDTH }
}


Review comment (suggestion):

The PR for stabilizing `cold_path` was just merged by the Rust team hours ago:
https://github.com/rust-lang/rust/pull/151576
Another PR for the `cold_path` release notes was just opened with the 1.95.0 milestone:
https://github.com/rust-lang/rust/issues/152251
Rust version 1.95.0 is planned to be stable on April 16, 2026:
https://releases.rs/docs/1.95.0/

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

use stdext::cold_path;

use super::Utf8Chars;
use super::tables::*;
use crate::document::ReadableDocument;
use crate::helpers::{CoordType, Point};

// On one hand it's disgusting that I wrote this as a global variable, but on the
// other hand, this isn't a public library API, and it makes the code a lot cleaner,
// because we don't need to inject this once-per-process value everywhere.
static mut AMBIGUOUS_WIDTH: usize = 1;

/// Sets the width of "ambiguous" width characters as per "UAX #11: East Asian Width".
///
/// Defaults to 1.
pub fn setup_ambiguous_width(ambiguous_width: CoordType) {
    unsafe { AMBIGUOUS_WIDTH = ambiguous_width as usize };
}

#[inline]
fn ambiguous_width() -> usize {
    // SAFETY: This is a global variable that is set once per process.
    // It is never changed after that, so this is safe to call.
    unsafe { AMBIGUOUS_WIDTH }
}
Repository: microsoft/edit
File: crates/edit/benches/lib.rs (Rust)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

use std::hint::black_box;
use std::io::Cursor;
use std::{mem, vec};

use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main};
use edit::helpers::*;
use edit::{buffer, glob, hash, json, oklab, simd, unicode};
use stdext::arena::{self, scratch_arena};
use stdext::collections::BVec;

struct EditingTracePatch<'a>(usize, usize, &'a str);

struct EditingTraceTransaction<'a> {
    patches: BVec<'a, EditingTracePatch<'a>>,
}

struct EditingTraceData<'a> {
    start_content: &'a str,
    end_content: &'a str,
    txns: BVec<'a, EditingTraceTransaction<'a>>,
}

fn bench_buffer(c: &mut Criterion) {
    let scratch = scratch_arena(None);
    let data = {
        let data = include_bytes!("../../../assets/editing-traces/rustcode.json.zst");
        let data = zstd::decode_all(Cursor::new(data)).unwrap();
        let data = str::from_utf8(&data).unwrap();

        let data = json::parse(&scratch, data).unwrap();
        let root = data.as_object().unwrap();
        let txns = root.get_array("txns").unwrap();

        let mut res = EditingTraceData {
            start_content: root.get_str("startContent").unwrap(),

Review comment (suggestion):
This moves the `Utf8Chars` into `stdext` so we can reuse it for `push_encode_utf16`. Hence there's some extra changes here.

`BVec::push_encode_utf16`: Direct translation from UTF8 to UTF16 (useful on Windows)
`Utf8Chars`: A parser which works directly on `[u8]` instead of `str`


Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

use std::hint::black_box;
use std::io::Cursor;
use std::{mem, vec};

use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main};
use edit::helpers::*;
use edit::{buffer, glob, hash, json, oklab, simd, unicode};
use stdext::arena::{self, scratch_arena};
use stdext::collections::BVec;
use stdext::unicode::Utf8Chars;

struct EditingTracePatch<'a>(usize, usize, &'a str);

struct EditingTraceTransaction<'a> {
    patches: BVec<'a, EditingTracePatch<'a>>,
}

struct EditingTraceData<'a> {
    start_content: &'a str,
    end_content: &'a str,
    txns: BVec<'a, EditingTraceTransaction<'a>>,
}

fn bench_buffer(c: &mut Criterion) {
    let scratch = scratch_arena(None);
    let data = {
        let data = include_bytes!("../../../assets/editing-traces/rustcode.json.zst");
        let data = zstd::decode_all(Cursor::new(data)).unwrap();
        let data = str::from_utf8(&data).unwrap();

        let data = json::parse(&scratch, data).unwrap();
        let root = data.as_object().unwrap();
        let txns = root.get_array("txns").unwrap();

        let mut res = EditingTraceData {
Repository: moyix/fauxpilot
File: launch.sh (Shell)

Code before:
#!/usr/bin/env bash

# Read in config.env file; error if not found
if [ ! -f config.env ]; then
    echo "config.env not found, please run setup.sh"
    exit 1
fi
source config.env

export NUM_GPUS=${NUM_GPUS}
export MODEL_DIR="${MODEL_DIR}"/"${MODEL}-${NUM_GPUS}gpu"
export GPUS=$(seq 0 $(( NUM_GPUS - 1 )) | paste -sd ',')

# On newer versions, docker-compose is docker compose
if command -v docker-compose > /dev/null; then
    docker compose up
else
    docker-compose up
fi


Review comment (suggestion):
I was having issues with `orphan` containers, as well as the `command -v docker-compose > /dev/null; then` resulting in `docker compose` leading to `docker: 'compose' is not a docker command.` 

Code after:
#!/usr/bin/env bash

# Read in .env file; error if not found
if [ ! -f .env ]; then
    echo ".env not found, running setup.sh"
    bash setup.sh
fi
source .env

# On newer versions, docker-compose is docker compose
docker compose up -d --remove-orphans || docker-compose up -d --remove-orphans
Repository: mxrch/GHunt
File: lib/utils.py (Python)

Code before:
    else:
        town = location["municipality"]
    location["town"] = town
    return location

def get_driverpath():
    if Os().wsl or Os().windows:
        driverpath = "./chromedriver.exe"
    else:
        driverpath = "./chromedriver"
    
    if isfile(driverpath):
        return driverpath
    else:
        exit("The chromedriver is missing.\nPlease put it in the GHunt directory.")

def get_wsl_chrome_options_args():
    chrome_options = Options()
    if Os().wsl or Os().windows:
        chrome_options.add_argument("--disable-gpu")
        chrome_options.add_argument("--disable-dev-shm-usage")
        chrome_options.add_argument("--disable-setuid-sandbox")
        chrome_options.add_argument("--no-first-run")
        chrome_options.add_argument("--no-zygote")
        chrome_options.add_argument("--single-process")
        chrome_options.add_argument("--no-sandbox")
        chrome_options.add_argument("--disable-features=VizDisplayCompositor")

Review comment (suggestion):
On some parts of the code, the `--no-sandbox` argument is added before `get_wsl_chrome_options_args` is called: Such cases are line 45-48 on `lib/photos.py`, and lines 64-67 on `lib/gmaps.py`.

`--no-sandbox` should always be added regardless of platform (much like the "headless" option).
I recommend you move it so it's before the `if Os().wsl or Os().windows:`

Code after:
    else:
        town = location["municipality"]
    location["town"] = town
    return location

def get_driverpath():
    if Os().wsl or Os().windows:
        driverpath = "./chromedriver.exe"
    else:
        driverpath = "./chromedriver"
    
    if isfile(driverpath):
        return driverpath
    else:
        exit("The chromedriver is missing.\nPlease put it in the GHunt directory.")

def get_chrome_options_args():
    chrome_options = Options()
    chrome_options.add_argument("--headless")
    chrome_options.add_argument("--no-sandbox")
    if Os().wsl or Os().windows:
        chrome_options.add_argument("--disable-gpu")
        chrome_options.add_argument("--disable-dev-shm-usage")
        chrome_options.add_argument("--disable-setuid-sandbox")
        chrome_options.add_argument("--no-first-run")
        chrome_options.add_argument("--no-zygote")
        chrome_options.add_argument("--single-process")
        chrome_options.add_argument("--disable-features=VizDisplayCompositor")
Repository: neherlab/covid19_scenarios
File: src/components/Main/Results/AgeBarChart.tsx (TypeScript)

Code before:
import React from 'react'

import ReactResizeDetector from 'react-resize-detector'

import { Bar, BarChart, CartesianGrid, Legend, Tooltip, XAxis, YAxis } from 'recharts'

import { AlgorithmResult } from '../../../algorithms/types/Result.types'

import { SeverityTableRow } from '../Scenario/SeverityTable'

import { colors } from './DeterministicLinePlot'

import { useTranslation } from 'react-i18next'
import { TFunction as ReactI18NextTFunction, TFunctionResult } from 'i18next'

const ASPECT_RATIO = 16 / 4

export interface SimProps {
  data?: AlgorithmResult
  rates?: SeverityTableRow[]
}

export function AgeBarChart({ data, rates }: SimProps) {
  if (!data || !rates) {
    return null
  }

  /**
   * `react-i18next`'s translation function (`useTranslation().t`) returns
   * `TFunctionResult` (i.e. `string | object | Array<string | object> | undefined | null`),
   * but `rechart`'s components only accept `string | number | undefined` for their `name`s, `label`s, .etc,
   * so extend `TFunction` with a signature that returns _just_ `string | number | undefined`
   */
  interface TFunction extends ReactI18NextTFunction {
    <TResult extends TFunctionResult = string,>(key: TResult): Extract<TFunctionResult, string | number | undefined>
  }

Review comment (suggestion):
It is basically a complicated way to cast return value to a more narrow type.
But what if  someone on the other end of the translation pipe goes crazy and `t()` will actually return an object or an array? Everything will blow up.

I think this should be a runtime check with a throw. Not elegant, but safe.
Feel free to either one-off inline or to wrap into a function (`safeT()`? `tString()`? idk) with the correct return type, both compile-time and run-time.


Code after:
import React from 'react'

import ReactResizeDetector from 'react-resize-detector'

import { Bar, BarChart, CartesianGrid, Legend, Tooltip, XAxis, YAxis } from 'recharts'

import { AlgorithmResult } from '../../../algorithms/types/Result.types'

import { SeverityTableRow } from '../Scenario/SeverityTable'

import { colors } from './DeterministicLinePlot'

import { useTranslation } from 'react-i18next'

const ASPECT_RATIO = 16 / 4

export interface SimProps {
  data?: AlgorithmResult
  rates?: SeverityTableRow[]
}

export function AgeBarChart({ data, rates }: SimProps) {
  if (!data || !rates) {
    return null
  }

  const { t: unsafeT } = useTranslation()
  const t = (...args: Parameters<typeof unsafeT>) => {
    const translation = unsafeT(...args)
    if (typeof translation === 'string' || typeof translation === 'undefined') {
      return translation
    }

    (process.env.NODE_ENV !== 'production') && console.warn('Translation incomatible in AgeBarChart.tsx', ...args)
    return String(translation)
  }
Repository: neherlab/covid19_scenarios
File: cypress/integration/scenarios.spec.ts (TypeScript)

Code before:
/// <reference types="cypress" />

import scenarios from '../../src/assets/data/scenarios/scenarios.json'

const scenariosKeys = Object.keys(scenarios);
// testing all the scenarios would be extremely long
const firstTenScenarios = scenariosKeys.slice(0, 10);

context('Scenario selector', () => {
  before(() => {
    cy.visit(Cypress.env('BASE_URL'))
    cy.closeDisclaimer()
  })

  beforeEach(() => {
    cy.get('#scenarioName').as('ScenarioDropdown')
    cy.get('.card--population').as('PopulationCard')
    cy.get('.card--epidemiology').as('EpidemiologyCard')
  })

  describe('The scenario selector', () => {
    it('should be present', () => {
      cy.get('@ScenarioDropdown')
        .should('be.visible')
    })

    it('should open when clicking on it', () => {
      cy.get('@ScenarioDropdown')
        .click()
        .find('[class$="-menu"]')
        .should('exist')
    })

Review comment (refactor):
There're **237** scenarios, running test on all of them will be time consuming, but if it' necessary we can remove this line and directly use `scenariosKeys` on line 49.

Code after:
/// <reference types="cypress" />

import scenarios from '../../src/assets/data/scenarios/scenarios.json'

const scenariosKeys = Object.keys(scenarios)
// testing all the scenarios would be extremely long
const firstTenScenarios = scenariosKeys.slice(0, 10)

context('Scenario selector', () => {
  before(() => {
    cy.visit(Cypress.env('BASE_URL'))
    cy.closeDisclaimer()
  })

  beforeEach(() => {
    cy.get('#scenarioName').as('ScenarioDropdown')
    cy.get('.card--population').as('PopulationCard')
    cy.get('.card--epidemiology').as('EpidemiologyCard')
  })

  describe('The scenario selector', () => {
    it('should be present', () => {
      cy.get('@ScenarioDropdown').should('be.visible')
    })

    it('should open when clicking on it', () => {
      cy.get('@ScenarioDropdown').click().find('[class$="-menu"]').should('exist')
    })

    it(`should have ${scenariosKeys.length} options`, () => {
      cy.get('@ScenarioDropdown')
        .find('[class$="-menu"]')