ronantakizawa/codereview-bench huggingface dataset:
Repository: leptos-rs/leptos
File: router/src/matching/horizontal/static_segment.rs (Rust)

Code before:
use super::{PartialPathMatch, PathSegment, PossibleRouteMatch};
use core::iter;
use std::{borrow::Cow, fmt::Debug};

impl PossibleRouteMatch for () {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        Some(PartialPathMatch::new(path, iter::empty(), ""))
    }

    fn generate_path(&self, _path: &mut Vec<PathSegment>) {}
}

pub trait Routable: Debug + Clone {
    fn into_str(&self) -> &'static str;
}

impl Routable for &'static str {
    fn into_str(&self) -> &'static str {
        self
    }
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct StaticSegment<T: Routable>(pub T);

impl<T: Routable> PossibleRouteMatch for StaticSegment<T> {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        let mut matched_len = 0;
        let mut test = path.chars().peekable();
        let mut this = self.0.into_str().chars();
        let mut has_matched =
            self.0.into_str().is_empty() || self.0.into_str() == "/";

        // match an initial /

Review comment (suggestion):
I'd recommend `as_path()` as a name for this method: `as_` in line with the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv) for a lightweight `&`-to-`&` conversion, and `as_path()` to give a little more context than `as_str()`.

Code after:
use super::{PartialPathMatch, PathSegment, PossibleRouteMatch};
use core::iter;
use std::{borrow::Cow, fmt::Debug};

impl PossibleRouteMatch for () {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        Some(PartialPathMatch::new(path, iter::empty(), ""))
    }

    fn generate_path(&self, _path: &mut Vec<PathSegment>) {}
}

pub trait AsPath {
    fn as_path(&self) -> &'static str;
}

impl AsPath for &'static str {
    fn as_path(&self) -> &'static str {
        self
    }
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct StaticSegment<T: AsPath>(pub T);

impl<T: AsPath> PossibleRouteMatch for StaticSegment<T> {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        let mut matched_len = 0;
        let mut test = path.chars().peekable();
        let mut this = self.0.as_path().chars();
        let mut has_matched =
            self.0.as_path().is_empty() || self.0.as_path() == "/";

        // match an initial /
Repository: leptos-rs/leptos
File: router/src/matching/horizontal/static_segment.rs (Rust)

Code before:
use super::{PartialPathMatch, PathSegment, PossibleRouteMatch};
use core::iter;
use std::{borrow::Cow, fmt::Debug};

impl PossibleRouteMatch for () {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        Some(PartialPathMatch::new(path, iter::empty(), ""))
    }

    fn generate_path(&self, _path: &mut Vec<PathSegment>) {}
}

pub trait Routable: Debug + Clone {
    fn into_str(&self) -> &'static str;
}

impl Routable for &'static str {
    fn into_str(&self) -> &'static str {
        self
    }
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct StaticSegment<T: Routable>(pub T);

impl<T: Routable> PossibleRouteMatch for StaticSegment<T> {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        let mut matched_len = 0;
        let mut test = path.chars().peekable();
        let mut this = self.0.into_str().chars();
        let mut has_matched =
            self.0.into_str().is_empty() || self.0.into_str() == "/";


Review comment (bug):
For single-method traits, it's best to name it the same as the trait itself -- So given my proposal of `as_path`, I'd suggest `AsPath` for the trait name.

The `Debug + Clone` trait bounds are not necessary -- as long as they are implemented on your enum they do not need to be bounds on the trait.

Code after:
use super::{PartialPathMatch, PathSegment, PossibleRouteMatch};
use core::iter;
use std::{borrow::Cow, fmt::Debug};

impl PossibleRouteMatch for () {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        Some(PartialPathMatch::new(path, iter::empty(), ""))
    }

    fn generate_path(&self, _path: &mut Vec<PathSegment>) {}
}

pub trait AsPath {
    fn as_path(&self) -> &'static str;
}

impl AsPath for &'static str {
    fn as_path(&self) -> &'static str {
        self
    }
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct StaticSegment<T: AsPath>(pub T);

impl<T: AsPath> PossibleRouteMatch for StaticSegment<T> {
    type ParamsIter = iter::Empty<(Cow<'static, str>, String)>;

    fn test<'a>(
        &self,
        path: &'a str,
    ) -> Option<PartialPathMatch<'a, Self::ParamsIter>> {
        let mut matched_len = 0;
        let mut test = path.chars().peekable();
        let mut this = self.0.as_path().chars();
        let mut has_matched =
            self.0.as_path().is_empty() || self.0.as_path() == "/";
Repository: leptos-rs/leptos
File: any_spawner/tests/futures_runtime.rs (Rust)

Code before:

    let _ = Executor::init_futures_executor();
    let rc = Rc::new(());
    Executor::spawn_local(async {
        _ = rc;
    });
    Executor::spawn(async {});
}

#[cfg(feature = "futures-executor")]
#[test]
fn can_make_threaded_progress() {
    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        Arc,
    };

    let _ = Executor::init_futures_executor();
    let counter = Arc::new(AtomicUsize::new(0));
    Executor::spawn({
        let counter = Arc::clone(&counter);
        async move {
            assert_eq!(counter.fetch_add(1, Ordering::AcqRel), 0);
        }
    });
    futures::executor::block_on(Executor::tick());
    assert_eq!(counter.load(Ordering::Acquire), 1);
}
#[cfg(feature = "futures-executor")]
#[test]
fn can_make_local_progress() {
    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        Arc,
    };
    let _ = Executor::init_futures_executor();
    let counter = Arc::new(AtomicUsize::new(0));
    Executor::spawn_local({
        let counter = Arc::clone(&counter);
        async move {
            assert_eq!(counter.fetch_add(1, Ordering::AcqRel), 0);
            Executor::spawn_local(async {
                // Should not crash
            });
        }
    });
    Executor::poll_local();
    assert_eq!(counter.load(Ordering::Acquire), 1);
}


Review comment (suggestion):
I guess the fact that this test worked at all was a fluke. There's nothing in futures-executor that guarantees that one future resolves before another.

Time to either remove the test, or rewrite it to work 100% of the time.

Code after:

    let _ = Executor::init_futures_executor();
    let rc = Rc::new(());
    Executor::spawn_local(async {
        _ = rc;
    });
    Executor::spawn(async {});
}

#[cfg(feature = "futures-executor")]
#[test]
fn can_make_local_progress() {
    use std::sync::{
        atomic::{AtomicUsize, Ordering},
        Arc,
    };
    let _ = Executor::init_futures_executor();
    let counter = Arc::new(AtomicUsize::new(0));
    Executor::spawn_local({
        let counter = Arc::clone(&counter);
        async move {
            assert_eq!(counter.fetch_add(1, Ordering::AcqRel), 0);
            Executor::spawn_local(async {
                // Should not crash
            });
        }
    });
    Executor::poll_local();
    assert_eq!(counter.load(Ordering::Acquire), 1);
}
Repository: leptos-rs/leptos
File: tachys/src/html/style.rs (Rust)

Code before:
        let (name, value) = self;
        style.push_str(&name);
        style.push(':');
        style.push_str(&value);
        style.push(';');
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        el: &crate::renderer::types::Element,
    ) -> Self::State {
        let style = Rndr::style(el);
        (style, self.0, self.1)
    }

    fn build(self, el: &crate::renderer::types::Element) -> Self::State {
        let (name, value) = self;
        let style = Rndr::style(el);
        Rndr::set_css_property(&style, &name, &value);
        (style, name, value)
    }

    fn rebuild(self, state: &mut Self::State) {
        let (name, value) = self;
        // state.1 was the previous name, theoretically the css name could be changed:
        if name != state.1 {
            <(Arc<str>, Arc<str>) as IntoStyle>::reset(state);
        }
        let (style, _prev_name, prev_value) = state;
        if value != *prev_value {
            Rndr::set_css_property(style, &name, &value);
        }
        *prev_value = value;
    }

    fn into_cloneable(self) -> Self::Cloneable {
        self
    }

    fn into_cloneable_owned(self) -> Self::Cloneable {
        self
    }

    fn dry_resolve(&mut self) {}

    async fn resolve(self) -> Self::AsyncOutput {
        self
    }

    /// Reset the renderer to the state before this style was added.
    fn reset(state: &mut Self::State) {

Review comment (suggestion):
If this is the case, then we should also `*prev_name = name`, or it will stick with the old name if this updates again.

Code after:
        let (name, value) = self;
        style.push_str(&name);
        style.push(':');
        style.push_str(&value);
        style.push(';');
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        el: &crate::renderer::types::Element,
    ) -> Self::State {
        let style = Rndr::style(el);
        (style, self.0, self.1)
    }

    fn build(self, el: &crate::renderer::types::Element) -> Self::State {
        let (name, value) = self;
        let style = Rndr::style(el);
        Rndr::set_css_property(&style, &name, &value);
        (style, name, value)
    }

    fn rebuild(self, state: &mut Self::State) {
        let (name, value) = self;
        // state.1 was the previous name, theoretically the css name could be changed:
        if name != state.1 {
            <(Arc<str>, Arc<str>) as IntoStyle>::reset(state);
        }
        let (style, prev_name, prev_value) = state;
        if value != *prev_value {
            Rndr::set_css_property(style, &name, &value);
        }
        *prev_name = name;
        *prev_value = value;
    }

    fn into_cloneable(self) -> Self::Cloneable {
        self
    }

    fn into_cloneable_owned(self) -> Self::Cloneable {
        self
    }

    fn dry_resolve(&mut self) {}

    async fn resolve(self) -> Self::AsyncOutput {
        self
    }

    /// Reset the renderer to the state before this style was added.
Repository: leptos-rs/leptos
File: tachys/src/html/style.rs (Rust)

Code before:
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        el: &crate::renderer::types::Element,
    ) -> Self::State {
        let style = Rndr::style(el);
        (style, self.0, self.1)
    }

    fn build(self, el: &crate::renderer::types::Element) -> Self::State {
        let (name, value) = self;
        let style = Rndr::style(el);
        Rndr::set_css_property(&style, name, value);
        (style, self.0, self.1)
    }

    fn rebuild(self, state: &mut Self::State) {
        let (name, value) = self;
        // state.1 was the previous name, theoretically the css name could be changed:
        if name != state.1 {
            <(&'a str, &'a str) as IntoStyle>::reset(state);
        }
        let (style, _prev_name, prev_value) = state;
        if value != *prev_value {
            Rndr::set_css_property(style, name, value);
        }
        *prev_value = value;
    }

    fn into_cloneable(self) -> Self::Cloneable {
        self
    }

    fn into_cloneable_owned(self) -> Self::CloneableOwned {
        (self.0.into(), self.1.into())
    }

    fn dry_resolve(&mut self) {}

    async fn resolve(self) -> Self::AsyncOutput {
        self
    }

    /// Reset the renderer to the state before this style was added.
    fn reset(state: &mut Self::State) {
        let (style, name, _value) = state;
        Rndr::remove_css_property(style, name);
    }
}


Review comment (suggestion):
See above: same thing about setting `prev_name`  to the new ~~value~~ name. *(I meant "new value of `name` but "new value" was a confusing way to write that.)*

Code after:
        style.push(';');
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        el: &crate::renderer::types::Element,
    ) -> Self::State {
        let style = Rndr::style(el);
        (style, self.0, self.1)
    }

    fn build(self, el: &crate::renderer::types::Element) -> Self::State {
        let (name, value) = self;
        let style = Rndr::style(el);
        Rndr::set_css_property(&style, name, value);
        (style, self.0, self.1)
    }

    fn rebuild(self, state: &mut Self::State) {
        let (name, value) = self;
        // state.1 was the previous name, theoretically the css name could be changed:
        if name != state.1 {
            <(&'a str, &'a str) as IntoStyle>::reset(state);
        }
        let (style, prev_name, prev_value) = state;
        if value != *prev_value {
            Rndr::set_css_property(style, name, value);
        }
        *prev_name = name;
        *prev_value = value;
    }

    fn into_cloneable(self) -> Self::Cloneable {
        self
    }

    fn into_cloneable_owned(self) -> Self::CloneableOwned {
        (self.0.into(), self.1.into())
    }

    fn dry_resolve(&mut self) {}

    async fn resolve(self) -> Self::AsyncOutput {
        self
    }

    /// Reset the renderer to the state before this style was added.
    fn reset(state: &mut Self::State) {
        let (style, name, _value) = state;
        Rndr::remove_css_property(style, name);
    }
Repository: leptos-rs/leptos
File: tachys/src/view/iterators.rs (Rust)

Code before:

impl<T, const N: usize> RenderHtml for [T; N]
where
    T: RenderHtml,
{
    type AsyncOutput = [T::AsyncOutput; N];

    const MIN_LENGTH: usize = 0;

    fn dry_resolve(&mut self) {
        for inner in self.iter_mut() {
            inner.dry_resolve();
        }
    }

    async fn resolve(self) -> Self::AsyncOutput {
        futures::future::join_all(self.into_iter().map(T::resolve))
            .await
            .into_iter()
            .collect::<Vec<_>>()
            .try_into()
            .unwrap_or_else(|_| unreachable!())
    }

    fn html_len(&self) -> usize {
        self.iter().map(RenderHtml::html_len).sum::<usize>() + 2
    }

    fn to_html_with_buf(
        self,
        buf: &mut String,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) {
        for child in self.into_iter() {
            child.to_html_with_buf(buf, position, escape, mark_branches);
        }
        buf.push_str("<!>");
    }

    fn to_html_async_with_buf<const OUT_OF_ORDER: bool>(
        self,
        buf: &mut StreamBuilder,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) where
        Self: Sized,
    {
        for child in self.into_iter() {

Review comment (suggestion):
Here I removed 1 from the constant compared to the Vec implementation. I did this guessing that the placeholder was counted in. But I have no idea what the constant is for.

Code after:

impl<T, const N: usize> RenderHtml for [T; N]
where
    T: RenderHtml,
{
    type AsyncOutput = [T::AsyncOutput; N];

    const MIN_LENGTH: usize = 0;

    fn dry_resolve(&mut self) {
        for inner in self.iter_mut() {
            inner.dry_resolve();
        }
    }

    async fn resolve(self) -> Self::AsyncOutput {
        futures::future::join_all(self.into_iter().map(T::resolve))
            .await
            .into_iter()
            .collect::<Vec<_>>()
            .try_into()
            .unwrap_or_else(|_| unreachable!())
    }

    fn html_len(&self) -> usize {
        self.iter().map(RenderHtml::html_len).sum::<usize>()
    }

    fn to_html_with_buf(
        self,
        buf: &mut String,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) {
        for child in self.into_iter() {
            child.to_html_with_buf(buf, position, escape, mark_branches);
        }
    }

    fn to_html_async_with_buf<const OUT_OF_ORDER: bool>(
        self,
        buf: &mut StreamBuilder,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) where
        Self: Sized,
    {
        for child in self.into_iter() {
            child.to_html_async_with_buf::<OUT_OF_ORDER>(
Repository: leptos-rs/leptos
File: tachys/src/view/iterators.rs (Rust)

Code before:
    }

    async fn resolve(self) -> Self::AsyncOutput {
        futures::future::join_all(self.into_iter().map(T::resolve))
            .await
            .into_iter()
            .collect::<Vec<_>>()
            .try_into()
            .unwrap_or_else(|_| unreachable!())
    }

    fn html_len(&self) -> usize {
        self.iter().map(RenderHtml::html_len).sum::<usize>() + 2
    }

    fn to_html_with_buf(
        self,
        buf: &mut String,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) {
        for child in self.into_iter() {
            child.to_html_with_buf(buf, position, escape, mark_branches);
        }
        buf.push_str("<!>");
    }

    fn to_html_async_with_buf<const OUT_OF_ORDER: bool>(
        self,
        buf: &mut StreamBuilder,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) where
        Self: Sized,
    {
        for child in self.into_iter() {
            child.to_html_async_with_buf::<OUT_OF_ORDER>(
                buf,
                position,
                escape,
                mark_branches,
            );
        }
        buf.push_sync("<!>");
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        cursor: &Cursor,

Review comment (bug):
Inserting the placeholder here will cause an issue, as it's not used during hydration (so children after this will hydrate incorrectly because there's an extra node

Code after:
    }

    async fn resolve(self) -> Self::AsyncOutput {
        futures::future::join_all(self.into_iter().map(T::resolve))
            .await
            .into_iter()
            .collect::<Vec<_>>()
            .try_into()
            .unwrap_or_else(|_| unreachable!())
    }

    fn html_len(&self) -> usize {
        self.iter().map(RenderHtml::html_len).sum::<usize>()
    }

    fn to_html_with_buf(
        self,
        buf: &mut String,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) {
        for child in self.into_iter() {
            child.to_html_with_buf(buf, position, escape, mark_branches);
        }
    }

    fn to_html_async_with_buf<const OUT_OF_ORDER: bool>(
        self,
        buf: &mut StreamBuilder,
        position: &mut Position,
        escape: bool,
        mark_branches: bool,
    ) where
        Self: Sized,
    {
        for child in self.into_iter() {
            child.to_html_async_with_buf::<OUT_OF_ORDER>(
                buf,
                position,
                escape,
                mark_branches,
            );
        }
    }

    fn hydrate<const FROM_SERVER: bool>(
        self,
        cursor: &Cursor,
        position: &PositionState,
    ) -> Self::State {
Repository: leptos-rs/leptos
File: reactive_graph/src/computed/selector.rs (Rust)

Code before:
                            || (prev.is_some()
                                && f(key, prev.as_ref().unwrap()))
                        {
                            signal.update(|n| *n = true);
                        }
                    }
                }
                next_value
            }
        }));

        Selector { subs, v, f, effect }
    }

    /// Reactively checks whether the given key is selected.
    pub fn selected(&self, key: &T) -> bool {
        let read = {
            let sub = self.subs.read().or_poisoned().get(key).cloned();
            sub.unwrap_or_else(|| {
                let signal = ArcRwSignal::new(false);
                self.subs
                    .write()
                    .or_poisoned()
                    .insert(key.clone(), signal.clone());
                signal
            })
        };
        read.track();
        (self.f)(key, self.v.read().or_poisoned().as_ref().unwrap())
    }

    /// Removes the listener for the given key.
    pub fn remove(&self, key: &T) {
        let mut subs = self.subs.write().or_poisoned();
        subs.remove(key);
    }

    /// Clears the listeners for all keys.
    pub fn clear(&self) {
        let mut subs = self.subs.write().or_poisoned();
        subs.clear();
    }
}


Review comment (bug):
There is a potential race condition between two thread running this code: both could read `None` and enter the `unwrap_or_else`, then the second thread to take the write lock and run the `insert` will overwrite the insertion done by the first one.

Using `entry(...).or_insert_with(...)` inside the `unwrap_or_else` should fix this, since that will not overwrite existing keys.

Code after:
                            || (prev.is_some()
                                && f(key, prev.as_ref().unwrap()))
                        {
                            signal.update(|n| *n = true);
                        }
                    }
                }
                next_value
            }
        }));

        Selector { subs, v, f, effect }
    }

    /// Reactively checks whether the given key is selected.
    pub fn selected(&self, key: &T) -> bool {
        let read = {
            let sub = self.subs.read().or_poisoned().get(key).cloned();
            sub.unwrap_or_else(|| {
                self.subs
                    .write()
                    .or_poisoned()
                    .entry(key.clone())
                    .or_insert_with(|| ArcRwSignal::new(false))
                    .clone()
            })
        };
        read.track();
        (self.f)(key, self.v.read().or_poisoned().as_ref().unwrap())
    }

    /// Removes the listener for the given key.
    pub fn remove(&self, key: &T) {
        let mut subs = self.subs.write().or_poisoned();
        subs.remove(key);
    }

    /// Clears the listeners for all keys.
    pub fn clear(&self) {
        let mut subs = self.subs.write().or_poisoned();
        subs.clear();
    }
}