ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/constants.js (JavaScript)

Code before:
 * Tells that we marked this derived and its reactions as visited during the "mark as (maybe) dirty"-phase.
 * Will be lifted during execution of the derived and during checking its dirty state (both are necessary
 * because a derived might be checked but not executed).
 */
export const WAS_MARKED = 1 << 16;

// Flags used for async
export const REACTION_IS_UPDATING = 1 << 21;
export const ASYNC = 1 << 22;

export const ERROR_VALUE = 1 << 23;

export const STATE_SYMBOL = Symbol('$state');
export const LEGACY_PROPS = Symbol('legacy props');
export const LOADING_ATTR_SYMBOL = Symbol('');
export const PROXY_PATH_SYMBOL = Symbol('proxy path');

/** allow users to ignore aborted signal errors if `reason.name === 'StaleReactionError` */
export const STALE_REACTION = new (class StaleReactionError extends Error {
	name = 'StaleReactionError';
	message = 'The reaction that called `getAbortSignal()` was re-run or destroyed';
})();

export const IS_XHTML =
	!!globalThis.document?.contentType &&
	/* @__PURE__ */ globalThis.document.contentType.includes('xml');
export const ELEMENT_NODE = 1;
export const TEXT_NODE = 3;
export const COMMENT_NODE = 8;
export const DOCUMENT_FRAGMENT_NODE = 11;


Review comment (bug):
```suggestion
	// We gotta write it like this because after downleveling the pure comment may end up in the wrong location
	!!globalThis.document?.contentType &&
```

Code after:
 * Tells that we marked this derived and its reactions as visited during the "mark as (maybe) dirty"-phase.
 * Will be lifted during execution of the derived and during checking its dirty state (both are necessary
 * because a derived might be checked but not executed).
 */
export const WAS_MARKED = 1 << 16;

// Flags used for async
export const REACTION_IS_UPDATING = 1 << 21;
export const ASYNC = 1 << 22;

export const ERROR_VALUE = 1 << 23;

export const STATE_SYMBOL = Symbol('$state');
export const LEGACY_PROPS = Symbol('legacy props');
export const LOADING_ATTR_SYMBOL = Symbol('');
export const PROXY_PATH_SYMBOL = Symbol('proxy path');

/** allow users to ignore aborted signal errors if `reason.name === 'StaleReactionError` */
export const STALE_REACTION = new (class StaleReactionError extends Error {
	name = 'StaleReactionError';
	message = 'The reaction that called `getAbortSignal()` was re-run or destroyed';
})();

export const IS_XHTML =
	// We gotta write it like this because after downleveling the pure comment may end up in the wrong location
	!!globalThis.document?.contentType &&
	/* @__PURE__ */ globalThis.document.contentType.includes('xml');
export const ELEMENT_NODE = 1;
export const TEXT_NODE = 3;
export const COMMENT_NODE = 8;
export const DOCUMENT_FRAGMENT_NODE = 11;
Repository: swc-project/swc
File: crates/swc_ecma_minifier/src/js.rs (Rust)

Code before:
}

impl Default for JsMinifyFormatOptions {
    fn default() -> Self {
        // Well, this should be a macro IMHO, but it's not so let's just use hacky way.
        serde_json::from_str("{}").unwrap()
    }
}

fn default_comments() -> BoolOrDataConfig<JsMinifyCommentOption> {
    BoolOrDataConfig::from_bool(false)
}

#[derive(Debug, Clone, Serialize, Deserialize, Default)]
pub enum JsMinifyCommentOption {
    #[serde(rename = "some")]
    #[default]
    PreserveSomeComments,
    #[serde(rename = "all")]
    PreserveAllComments,
    #[serde(untagged)]
    PreserveRegexComments(Box<PreserveRegexCommentsInner>),
}

#[derive(Debug, Clone)]
pub struct PreserveRegexCommentsInner {
    regex: CachedJsRegex,
    origin: String,
}

impl PreserveRegexCommentsInner {
    pub fn new(regex_str: String) -> Option<Self> {
        let regex = CachedJsRegex::new(&regex_str).ok()?;
        Some(Self {
            regex,
            origin: regex_str,
        })
    }

    pub fn is_match(&self, haystack: &str) -> bool {
        self.regex.find(haystack).is_some()
    }
}

impl Serialize for PreserveRegexCommentsInner {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        let mut map = serializer.serialize_map(Some(1))?;
        map.serialize_entry("regex", &self.origin)?;

Review comment (performance):
But if we have serialize/deserialize implementation on `CachedJsRegex` and the original source regex in `CachedJsRegex` this type would be much simpler

Code after:
impl Default for JsMinifyFormatOptions {
    fn default() -> Self {
        // Well, this should be a macro IMHO, but it's not so let's just use hacky way.
        serde_json::from_str("{}").unwrap()
    }
}

fn default_comments() -> BoolOrDataConfig<JsMinifyCommentOption> {
    BoolOrDataConfig::from_bool(false)
}

#[derive(Debug, Clone, Serialize, Deserialize, Default)]
pub enum JsMinifyCommentOption {
    #[serde(rename = "some")]
    #[default]
    PreserveSomeComments,
    #[serde(rename = "all")]
    PreserveAllComments,
    #[serde(untagged)]
    PreserveRegexComments { regex: CachedJsRegex },
}

fn default_module() -> IsModule {
    IsModule::Bool(false)
}
Repository: swc-project/swc
File: crates/swc_ecma_lexer/src/common/syntax.rs (Rust)

Code before:
            flags |= EsSyntaxFlags::DECORATORS_BEFORE_EXPORT;
        }
        if self.export_default_from {
            flags |= EsSyntaxFlags::EXPORT_DEFAULT_FROM;
        }
        if self.import_attributes {
            flags |= EsSyntaxFlags::IMPORT_ATTRIBUTES;
        }
        if self.allow_super_outside_method {
            flags |= EsSyntaxFlags::ALLOW_SUPER_OUTSIDE_METHOD;
        }
        if self.allow_return_outside_function {
            flags |= EsSyntaxFlags::ALLOW_RETURN_OUTSIDE_FUNCTION;
        }
        if self.auto_accessors {
            flags |= EsSyntaxFlags::AUTO_ACCESSORS;
        }
        if self.explicit_resource_management {
            flags |= EsSyntaxFlags::EXPLICIT_RESOURCE_MANAGEMENT;
        }
        flags
    }
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SyntaxFlags {
    Es(EsSyntaxFlags),
    #[cfg(feature = "typescript")]
    #[cfg_attr(docsrs, doc(cfg(feature = "typescript")))]
    Ts(TsSyntaxFlags),
}

impl Default for SyntaxFlags {
    fn default() -> Self {
        SyntaxFlags::Es(Default::default())
    }
}

impl SyntaxFlags {
    pub fn auto_accessors(self) -> bool {
        match self {
            #[cfg(feature = "typescript")]
            Self::Ts(_) => true,
            Self::Es(flags) => flags.contains(EsSyntaxFlags::AUTO_ACCESSORS),
        }
    }

    pub fn import_attributes(self) -> bool {
        match self {
            #[cfg(feature = "typescript")]
            Self::Ts(_) => true,

Review comment (suggestion):
I think it's better to merge two flag types as a single bitflag

Code after:
        if self.decorators_before_export {
            flags |= SyntaxFlags::DECORATORS_BEFORE_EXPORT;
        }
        if self.export_default_from {
            flags |= SyntaxFlags::EXPORT_DEFAULT_FROM;
        }
        if self.import_attributes {
            flags |= SyntaxFlags::IMPORT_ATTRIBUTES;
        }
        if self.allow_super_outside_method {
            flags |= SyntaxFlags::ALLOW_SUPER_OUTSIDE_METHOD;
        }
        if self.allow_return_outside_function {
            flags |= SyntaxFlags::ALLOW_RETURN_OUTSIDE_FUNCTION;
        }
        if self.auto_accessors {
            flags |= SyntaxFlags::AUTO_ACCESSORS;
        }
        if self.explicit_resource_management {
            flags |= SyntaxFlags::EXPLICIT_RESOURCE_MANAGEMENT;
        }
        flags
    }
}

impl SyntaxFlags {
    #[inline(always)]
    pub fn auto_accessors(&self) -> bool {
        self.contains(Self::AUTO_ACCESSORS)
    }

    #[inline(always)]
    pub fn import_attributes(&self) -> bool {
        true
    }

    /// Should we parse jsx?
    #[inline(always)]
    pub fn jsx(&self) -> bool {
        self.contains(SyntaxFlags::JSX)
    }

    #[inline(always)]
    pub fn fn_bind(&self) -> bool {
        self.contains(SyntaxFlags::FN_BIND)
    }

    #[inline(always)]
    pub fn decorators(&self) -> bool {
        self.contains(SyntaxFlags::DECORATORS)
    }
Repository: swc-project/swc
File: crates/hstr/src/dynamic.rs (Rust)

Code before:
impl Default for AtomStore {
    fn default() -> Self {
        const CAP: usize = 64;
        Self {
            arena: Vec::with_capacity(CAP),
            map: HashMap::with_capacity_and_hasher(CAP, BuildEntryHasher::default()),
        }
    }
}

impl AtomStore {
    #[inline(always)]
    pub fn atom<'a>(&mut self, text: impl Into<Cow<'a, str>>) -> Atom {
        atom_in(self, &text.into(), false)
    }

    fn gc(&mut self) {
        let mut new_arena = Vec::with_capacity(self.arena.len());
        for item in std::mem::take(&mut self.arena) {
            if ThinArc::strong_count(&item.0) == 1 {
                let prev = self.map.remove(&item.0.header.header.hash);
                debug_assert!(prev.is_some());
            } else {
                new_arena.push(item);
            }
        }
        self.arena = new_arena;
    }
}

thread_local! {
    static GLOBAL_DATA: RefCell<AtomStore> = Default::default();
}

pub fn global_atom_store_gc() {
    GLOBAL_DATA.with(|global| {
        let mut store = global.borrow_mut();
        store.gc();
    });
}

pub(crate) fn global_atom(text: &str) -> Atom {
    GLOBAL_DATA.with(|global| {
        let mut store = global.borrow_mut();

        atom_in(&mut *store, text, true)
    })
}

/// This can create any kind of [Atom], although this lives in the `dynamic`
/// module.

Review comment (bug):
The `gc` method replaces the arena vector without updating the stored indices in `map`, leading to stale or incorrect mappings. After rebuilding `arena`, you should also rebuild or update `map` entries to reflect each item's new index.
```suggestion
        }
        self.map.clear();
        for (index, item) in new_arena.iter().enumerate() {
            self.map.insert(item.0.header.header.hash, index);
        }
```

Code after:
        Self {
            data: hashbrown::HashMap::with_capacity_and_hasher(64, BuildEntryHasher::default()),
        }
    }
}

impl AtomStore {
    #[inline(always)]
    pub fn atom<'a>(&mut self, text: impl Into<Cow<'a, str>>) -> Atom {
        atom_in(self, &text.into(), false)
    }

    fn gc(&mut self) {
        self.data.retain(|item, _| {
            let count = ThinArc::strong_count(&item.0);
            debug_assert!(count > 0);
            count > 1
        });
    }
}

thread_local! {
    static GLOBAL_DATA: RefCell<AtomStore> = Default::default();
}

/// Cleans up atoms in the global store that are no longer referenced.
pub fn global_atom_store_gc() {
    GLOBAL_DATA.with(|global| {
        let mut store = global.borrow_mut();
        store.gc();
    });
}

pub(crate) fn global_atom(text: &str) -> Atom {
    GLOBAL_DATA.with(|global| {
        let mut store = global.borrow_mut();

        atom_in(&mut *store, text, true)
    })
}

/// This can create any kind of [Atom], although this lives in the `dynamic`
/// module.
fn atom_in<S>(storage: S, text: &str, is_global: bool) -> Atom
where
    S: Storage,
{
    let len = text.len();

    if len < MAX_INLINE_LEN {
        // INLINE_TAG ensures this is never zero
Repository: swc-project/swc
File: crates/hstr/src/dynamic.rs (Rust)

Code before:
    }

    fn write_u64(&mut self, val: u64) {
        #[cfg(debug_assertions)]
        {
            debug_assert!(
                !self.write_called,
                "EntryHasher expects write_u64 to be called only once",
            );
            self.write_called = true;
        }

        self.hash = val;
    }
}

#[cfg(test)]
mod tests {
    use crate::{dynamic::GLOBAL_DATA, global_atom_store_gc, Atom};

    fn expect_size(expected: usize) {
        // This is a helper function to count the number of bytes in the global store.
        GLOBAL_DATA.with(|global| {
            let store = global.borrow();
            assert!(store.arena.len() == expected);
            assert!(store.map.len() == expected);
        })
    }

    #[test]
    fn global_ref_count_dynamic_0() {
        expect_size(0);

        // The strings should be long enough so that they are not inline even under
        // feature `atom_size_128`
        let atom1 = Atom::new("Hello, beautiful world!");

        expect_size(1);

        let atom2 = Atom::new("Hello, beautiful world!");

        expect_size(1);

        // 2 for the two atoms, 1 for the global store
        assert_eq!(atom1.ref_count(), 3);
        assert_eq!(atom2.ref_count(), 3);

        drop(atom1);

        expect_size(1);


Review comment (bug):
[nitpick] Use `assert_eq!(store.arena.len(), expected)` instead of `assert!` for clearer failure diagnostics in tests.
```suggestion
            assert_eq!(store.arena.len(), expected);
            assert_eq!(store.map.len(), expected);
```

Code after:
            self.write_called = true;
        }

        self.hash = val;
    }
}

#[cfg(test)]
mod tests {
    use crate::{dynamic::GLOBAL_DATA, global_atom_store_gc, Atom};

    fn expect_size(expected: usize) {
        // This is a helper function to count the number of bytes in the global store.
        GLOBAL_DATA.with(|global| {
            let store = global.borrow();
            assert_eq!(store.data.len(), expected);
        })
    }

    #[test]
    fn global_ref_count_dynamic_0() {
        expect_size(0);

        // The strings should be long enough so that they are not inline even under
        // feature `atom_size_128`
        let atom1 = Atom::new("Hello, beautiful world!");

        expect_size(1);

        let atom2 = Atom::new("Hello, beautiful world!");

        expect_size(1);

        // 2 for the two atoms, 1 for the global store
        assert_eq!(atom1.ref_count(), 3);
        assert_eq!(atom2.ref_count(), 3);

        drop(atom1);

        expect_size(1);

        // 1 for the atom2, 1 for the global store
        assert_eq!(atom2.ref_count(), 2);

        drop(atom2);

        expect_size(1);
        global_atom_store_gc();
        expect_size(0);
    }
Repository: swc-project/swc
File: crates/swc_ecma_transforms_base/src/rename/mod.rs (Rust)

Code before:
        None
    }

    fn store_cache(&mut self, _update: &FxHashMap<Id, Self::Target>) {}

    /// Should increment `n`.
    fn new_name_for(&self, orig: &Id, n: &mut usize) -> Atom;

    fn unresolved_symbols(&self) -> Vec<Atom> {
        Default::default()
    }

    /// Return true if the identifier should be preserved.
    #[inline]
    fn preserve_name(&self, _orig: &Id) -> bool {
        false
    }
}

pub type RenameMap = FxHashMap<Id, Atom>;

pub fn rename(map: &RenameMap) -> impl '_ + Pass + VisitMut {
    rename_with_config(map, Default::default())
}

pub fn rename_with_config<V: RenamedVariable + std::marker::Sync>(
    map: &FxHashMap<Id, V>,
    config: Config,
) -> impl '_ + Pass + VisitMut {
    visit_mut_pass(Operator {
        rename: map,
        config,
        extra: Default::default(),
    })
}

pub fn renamer<R>(config: Config, renamer: R) -> impl Pass + VisitMut
where
    R: Renamer<Target = Atom>,
{
    visit_mut_pass(RenamePass {
        config,
        renamer,
        preserved: Default::default(),
        unresolved: Default::default(),
        previous_cache: Default::default(),
        total_map: None,
        marker: std::marker::PhantomData::<Atom>,
    })
}


Review comment (suggestion):
When using the imported `self::renamer_{concurrent,single}::Sync`, I got compile errors. Not sure how this is intended to work.

Code after:

    fn get_cached(&self) -> Option<Cow<FxHashMap<Id, Self::Target>>> {
        None
    }

    fn store_cache(&mut self, _update: &FxHashMap<Id, Self::Target>) {}

    /// Should increment `n`.
    fn new_name_for(&self, orig: &Id, n: &mut usize) -> Atom;

    fn unresolved_symbols(&self) -> Vec<Atom> {
        Default::default()
    }

    /// Return true if the identifier should be preserved.
    #[inline]
    fn preserve_name(&self, _orig: &Id) -> bool {
        false
    }
}

pub type RenameMap = FxHashMap<Id, Atom>;

pub fn rename<V: RenamedVariable>(map: &FxHashMap<Id, V>) -> impl '_ + Pass + VisitMut {
    rename_with_config(map, Default::default())
}

pub fn rename_with_config<V: RenamedVariable>(
    map: &FxHashMap<Id, V>,
    config: Config,
) -> impl '_ + Pass + VisitMut {
    visit_mut_pass(Operator {
        rename: map,
        config,
        extra: Default::default(),
    })
}

pub fn renamer<R>(config: Config, renamer: R) -> impl Pass + VisitMut
where
    R: Renamer<Target = Atom>,
{
    visit_mut_pass(RenamePass {
        config,
        renamer,
        preserved: Default::default(),
        unresolved: Default::default(),
        previous_cache: Default::default(),
        total_map: None,
        marker: std::marker::PhantomData::<Atom>,
    })
Repository: swc-project/swc
File: crates/swc_ecma_transforms_base/src/rename/ops.rs (Rust)

Code before:
            self.renamed
                .push(ExportSpecifier::Named(ExportNamedSpecifier {
                    span: i.span,
                    exported: Some(ModuleExportName::Ident(orig)),
                    orig: ModuleExportName::Ident(i.clone()),
                    is_type_only: false,
                }));
        }
    }
}

impl<V> Operator<'_, V>
where
    V: RenamedVariable,
{
    /// Returns `Ok(renamed_ident)` if ident should be renamed.
    fn rename_ident(&mut self, ident: &mut Ident) -> Result<(), ()> {
        if let Some(new_id) = self.rename.get(&ident.to_id()) {
            let (new_sym, new_ctxt) = new_id.to_id();

            if new_sym == ident.sym {
                return Err(());
            }

            ident.ctxt = new_ctxt;
            ident.sym = new_sym;
            return Ok(());
        }

        Err(())
    }
}


Review comment (suggestion):
This clone is still necessary since `new_sym` is a reference (`&Atom`) from the `atom()` method, but the optimization reduces clones elsewhere in the function by avoiding the `to_id()` decomposition.

Code after:
            self.renamed
                .push(ExportSpecifier::Named(ExportNamedSpecifier {
                    span: i.span,
                    exported: Some(ModuleExportName::Ident(orig)),
                    orig: ModuleExportName::Ident(i.clone()),
                    is_type_only: false,
                }));
        }
    }
}

impl<V> Operator<'_, V>
where
    V: RenamedVariable,
{
    /// Returns `Ok(renamed_ident)` if ident should be renamed.
    fn rename_ident(&mut self, ident: &mut Ident) -> Result<(), ()> {
        if let Some(new_id) = self.rename.get(&ident.to_id()) {
            let new_sym = new_id.atom();

            if ident.sym.eq(new_sym) {
                return Err(());
            }

            ident.ctxt = new_id.ctxt();
            ident.sym = new_sym.clone();
            return Ok(());
        }

        Err(())
    }
}
Repository: swc-project/swc
File: crates/swc_ecma_compat_es2015/src/spread.rs (Rust)

Code before:
use swc_ecma_visit::{
    noop_visit_mut_type, noop_visit_type, visit_mut_pass, Visit, VisitMut, VisitMutWith, VisitWith,
};
use swc_trace_macro::swc_trace;

pub fn spread(c: Config) -> impl Pass {
    visit_mut_pass(Spread {
        c,
        vars: Default::default(),
    })
}

#[derive(Debug, Clone, Copy, Default, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Config {
    pub loose: bool,
}

/// es2015 - `SpreadElement`
#[derive(Default)]
struct Spread {
    c: Config,
    vars: Vec<VarDeclarator>,
}

#[swc_trace]
#[fast_path(SpreadFinder)]
impl VisitMut for Spread {
    noop_visit_mut_type!(fail);

    fn visit_mut_module_items(&mut self, n: &mut Vec<ModuleItem>) {
        self.visit_mut_stmt_like(n);
    }

    fn visit_mut_stmts(&mut self, n: &mut Vec<Stmt>) {
        self.visit_mut_stmt_like(n);
    }

    fn visit_mut_expr(&mut self, e: &mut Expr) {
        e.visit_mut_children_with(self);

        match e {
            Expr::Array(ArrayLit { span, elems }) => {
                if !elems.iter().any(|e| {
                    matches!(
                        e,
                        Some(ExprOrSpread {
                            spread: Some(_),
                            ..
                        })
                    )

Review comment (suggestion):
The struct is missing the `#[derive(Default)]` attribute that was removed. This is a breaking change since `Default::default()` can no longer be used to create instances of this struct, but the struct is not publicly exposed so this should be acceptable.

Code after:
use swc_ecma_visit::{
    noop_visit_mut_type, noop_visit_type, visit_mut_pass, Visit, VisitMut, VisitMutWith, VisitWith,
};
use swc_trace_macro::swc_trace;

pub fn spread(c: Config, unresolved_mark: Mark) -> impl Pass {
    let unresolved_ctxt = SyntaxContext::empty().apply_mark(unresolved_mark);
    visit_mut_pass(Spread {
        c,
        unresolved_ctxt,
        vars: Default::default(),
    })
}

#[derive(Debug, Clone, Copy, Default, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Config {
    pub loose: bool,
}

/// es2015 - `SpreadElement`
struct Spread {
    c: Config,
    unresolved_ctxt: SyntaxContext,
    vars: Vec<VarDeclarator>,
}

#[swc_trace]
#[fast_path(SpreadFinder)]
impl VisitMut for Spread {
    noop_visit_mut_type!(fail);

    fn visit_mut_module_items(&mut self, n: &mut Vec<ModuleItem>) {
        self.visit_mut_stmt_like(n);
    }

    fn visit_mut_stmts(&mut self, n: &mut Vec<Stmt>) {
        self.visit_mut_stmt_like(n);
    }

    fn visit_mut_expr(&mut self, e: &mut Expr) {
        e.visit_mut_children_with(self);

        match e {
            Expr::Array(ArrayLit { span, elems }) => {
                if !elems.iter().any(|e| {
                    matches!(
                        e,
                        Some(ExprOrSpread {
                            spread: Some(_),
                            ..