ronantakizawa/codereview-bench huggingface dataset:
Repository: actix/actix-web
File: actix-http/src/extensions.rs (Rust)

Code before:
    /// Get a mutable reference to a type previously inserted on this `Extensions`.
    pub fn get_mut<T: 'static>(&mut self) -> Option<&mut T> {
        self.map
            .get_mut(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast_mut())
    }

    /// Remove a type from this `Extensions`.
    ///
    /// If a extension of this type existed, it will be returned.
    pub fn remove<T: 'static>(&mut self) -> Option<T> {
        self.map
            .remove(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast().ok().map(|boxed| *boxed))
    }

    /// Clear the `Extensions` of all inserted extensions.
    #[inline]
    pub fn clear(&mut self) {
        self.map.clear();
    }

    /// Extends it with the content for another `Extensions`
    pub fn extend(&mut self, extensions: Extensions) {
        self.map.extend(extensions.map);
    }
}

impl fmt::Debug for Extensions {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Extensions").finish()
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_remove() {
        let mut map = Extensions::new();

        map.insert::<i8>(123);
        assert!(map.get::<i8>().is_some());

        map.remove::<i8>();
        assert!(map.get::<i8>().is_none());
    }

    #[test]
    fn test_clear() {

Review comment (suggestion):
```suggestion
    /// Extends self with the items from another `Extensions`.
    pub fn extend(&mut self, other: Extensions) {
        self.map.extend(other.map);
    }
```

Code after:
    /// Get a mutable reference to a type previously inserted on this `Extensions`.
    pub fn get_mut<T: 'static>(&mut self) -> Option<&mut T> {
        self.map
            .get_mut(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast_mut())
    }

    /// Remove a type from this `Extensions`.
    ///
    /// If a extension of this type existed, it will be returned.
    pub fn remove<T: 'static>(&mut self) -> Option<T> {
        self.map
            .remove(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast().ok().map(|boxed| *boxed))
    }

    /// Clear the `Extensions` of all inserted extensions.
    #[inline]
    pub fn clear(&mut self) {
        self.map.clear();
    }

    /// Extends self with the items from another `Extensions`.
    pub fn extend(&mut self, other: Extensions) {
        self.map.extend(other.map);
    }
}

impl fmt::Debug for Extensions {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Extensions").finish()
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_remove() {
        let mut map = Extensions::new();

        map.insert::<i8>(123);
        assert!(map.get::<i8>().is_some());

        map.remove::<i8>();
        assert!(map.get::<i8>().is_none());
    }

    #[test]
    fn test_clear() {
Repository: actix/actix-web
File: src/config.rs (Rust)

Code before:

impl ServiceConfig {
    pub(crate) fn new() -> Self {
        Self {
            services: Vec::new(),
            data: Vec::new(),
            external: Vec::new(),
            extensions: Extensions::new(),
        }
    }

    /// Set application data. Application data could be accessed
    /// by using `Data<T>` extractor where `T` is data type.
    ///
    /// This is same as `App::data()` method.
    pub fn data<S: 'static>(&mut self, data: S) -> &mut Self {
        self.data.push(Box::new(Data::new(data)));
        self
    }

    /// Set application level arbitrary data item.
    ///
    /// Application data stored with `App::app_data()` method is available
    /// via `HttpRequest::app_data()` method at runtime.
    ///
    /// Note: this method is ignored for `Scope::configure` method
    pub fn app_data<U: 'static>(&mut self, ext: U) -> &mut Self {
        self.extensions.insert(ext);
        self
    }

    /// Configure route for a specific path.
    ///
    /// This is same as `App::route()` method.
    pub fn route(&mut self, path: &str, mut route: Route) -> &mut Self {
        self.service(
            Resource::new(path)
                .add_guards(route.take_guards())
                .route(route),
        )
    }

    /// Register http service.
    ///
    /// This is same as `App::service()` method.
    pub fn service<F>(&mut self, factory: F) -> &mut Self
    where
        F: HttpServiceFactory + 'static,
    {
        self.services
            .push(Box::new(ServiceFactoryWrapper::new(factory)));

Review comment (suggestion):
```suggestion
    /// Set arbitrary data item.
    ///
    /// This is same as `App::data()` method.
```

Code after:

impl ServiceConfig {
    pub(crate) fn new() -> Self {
        Self {
            services: Vec::new(),
            data: Vec::new(),
            external: Vec::new(),
            extensions: Extensions::new(),
        }
    }

    /// Set application data. Application data could be accessed
    /// by using `Data<T>` extractor where `T` is data type.
    ///
    /// This is same as `App::data()` method.
    pub fn data<S: 'static>(&mut self, data: S) -> &mut Self {
        self.data.push(Box::new(Data::new(data)));
        self
    }

    /// Set arbitrary data item.
    ///
    /// This is same as `App::data()` method.
    pub fn app_data<U: 'static>(&mut self, ext: U) -> &mut Self {
        self.extensions.insert(ext);
        self
    }

    /// Configure route for a specific path.
    ///
    /// This is same as `App::route()` method.
    pub fn route(&mut self, path: &str, mut route: Route) -> &mut Self {
        self.service(
            Resource::new(path)
                .add_guards(route.take_guards())
                .route(route),
        )
    }

    /// Register http service.
    ///
    /// This is same as `App::service()` method.
    pub fn service<F>(&mut self, factory: F) -> &mut Self
    where
        F: HttpServiceFactory + 'static,
    {
        self.services
            .push(Box::new(ServiceFactoryWrapper::new(factory)));
        self
    }
Repository: actix/actix-web
File: actix-http/src/extensions.rs (Rust)

Code before:
    }

    /// Get a mutable reference to a type previously inserted on this `Extensions`.
    pub fn get_mut<T: 'static>(&mut self) -> Option<&mut T> {
        self.map
            .get_mut(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast_mut())
    }

    /// Remove a type from this `Extensions`.
    ///
    /// If a extension of this type existed, it will be returned.
    pub fn remove<T: 'static>(&mut self) -> Option<T> {
        self.map
            .remove(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast().ok().map(|boxed| *boxed))
    }

    /// Clear the `Extensions` of all inserted extensions.
    #[inline]
    pub fn clear(&mut self) {
        self.map.clear();
    }

    /// Extends it with the content for another `Extensions`
    pub fn extend(&mut self, extensions: Extensions) {
        self.map.extend(extensions.map);
    }
}

impl fmt::Debug for Extensions {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Extensions").finish()
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_remove() {
        let mut map = Extensions::new();

        map.insert::<i8>(123);
        assert!(map.get::<i8>().is_some());

        map.remove::<i8>();
        assert!(map.get::<i8>().is_none());
    }


Review comment (suggestion):
Wouldn't implementing [`std::iter::Extend`](https://doc.rust-lang.org/std/iter/trait.Extend.html) make more sense?

Also, unit-tests for this functionality would be appreciated.

Code after:
    }

    /// Get a mutable reference to a type previously inserted on this `Extensions`.
    pub fn get_mut<T: 'static>(&mut self) -> Option<&mut T> {
        self.map
            .get_mut(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast_mut())
    }

    /// Remove a type from this `Extensions`.
    ///
    /// If a extension of this type existed, it will be returned.
    pub fn remove<T: 'static>(&mut self) -> Option<T> {
        self.map
            .remove(&TypeId::of::<T>())
            .and_then(|boxed| boxed.downcast().ok().map(|boxed| *boxed))
    }

    /// Clear the `Extensions` of all inserted extensions.
    #[inline]
    pub fn clear(&mut self) {
        self.map.clear();
    }

    /// Extends self with the items from another `Extensions`.
    pub fn extend(&mut self, other: Extensions) {
        self.map.extend(other.map);
    }
}

impl fmt::Debug for Extensions {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Extensions").finish()
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_remove() {
        let mut map = Extensions::new();

        map.insert::<i8>(123);
        assert!(map.get::<i8>().is_some());

        map.remove::<i8>();
        assert!(map.get::<i8>().is_none());
    }
Repository: actix/actix-web
File: actix-files/src/files.rs (Rust)

Code before:
    #[inline]
    pub fn disable_content_disposition(mut self) -> Self {
        self.file_flags.remove(named::Flags::CONTENT_DISPOSITION);
        self
    }

    /// Sets default handler which is used when no matched file could be found.
    pub fn default_handler<F, U>(mut self, f: F) -> Self
    where
        F: IntoServiceFactory<U>,
        U: ServiceFactory<
                Config = (),
                Request = ServiceRequest,
                Response = ServiceResponse,
                Error = Error,
            > + 'static,
    {
        // create and configure default resource
        self.default = Rc::new(RefCell::new(Some(Rc::new(boxed::factory(
            f.into_factory().map_init_err(|_| ()),
        )))));

        self
    }

    /// Enables serving hidden files and directories, allowing a trailing dots in url fragments.
    #[inline]
    pub fn use_hidden_files(mut self) -> Self {
        self.hidden_files = true;
        self
    }
}

impl HttpServiceFactory for Files {
    fn register(self, config: &mut AppService) {
        if self.default.borrow().is_none() {
            *self.default.borrow_mut() = Some(config.default_service());
        }

        let rdef = if config.is_root() {
            ResourceDef::root_prefix(&self.path)
        } else {
            ResourceDef::prefix(&self.path)
        };

        config.register_service(rdef, None, self, None)
    }
}

impl ServiceFactory for Files {
    type Request = ServiceRequest;

Review comment (suggestion):
```suggestion
    /// Enables serving hidden files and directories, allowing a leading dots in url fragments.
```

Code after:
    #[inline]
    pub fn disable_content_disposition(mut self) -> Self {
        self.file_flags.remove(named::Flags::CONTENT_DISPOSITION);
        self
    }

    /// Sets default handler which is used when no matched file could be found.
    pub fn default_handler<F, U>(mut self, f: F) -> Self
    where
        F: IntoServiceFactory<U>,
        U: ServiceFactory<
                Config = (),
                Request = ServiceRequest,
                Response = ServiceResponse,
                Error = Error,
            > + 'static,
    {
        // create and configure default resource
        self.default = Rc::new(RefCell::new(Some(Rc::new(boxed::factory(
            f.into_factory().map_init_err(|_| ()),
        )))));

        self
    }

    /// Enables serving hidden files and directories, allowing a leading dots in url fragments.
    #[inline]
    pub fn use_hidden_files(mut self) -> Self {
        self.hidden_files = true;
        self
    }
}

impl HttpServiceFactory for Files {
    fn register(self, config: &mut AppService) {
        if self.default.borrow().is_none() {
            *self.default.borrow_mut() = Some(config.default_service());
        }

        let rdef = if config.is_root() {
            ResourceDef::root_prefix(&self.path)
        } else {
            ResourceDef::prefix(&self.path)
        };

        config.register_service(rdef, None, self, None)
    }
}

impl ServiceFactory for Files {
    type Request = ServiceRequest;
Repository: actix/actix-web
File: actix-http/src/lib.rs (Rust)

Code before:
    pub use http::header::{HeaderName, HeaderValue};
    pub use http::uri::PathAndQuery;
    pub use http::{uri, Error, Uri};
    pub use http::{Method, StatusCode, Version};

    pub use crate::cookie::{Cookie, CookieBuilder};
    pub use crate::header::HeaderMap;

    /// Various http headers
    pub mod header {
        pub use crate::header::*;
    }
    pub use crate::header::ContentEncoding;
    pub use crate::message::ConnectionType;
}

/// Http protocol
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum Protocol {
    Http1,
    Http2,
}

type ConnectCallback<IO> = dyn Fn(&IO, &mut Extensions);

// container for data that extract with ConnectCallback
pub(crate) struct OnConnectData(Option<Extensions>);

impl Default for OnConnectData {
    fn default() -> Self {
        Self(None)
    }
}

impl OnConnectData {
    // construct self from io.
    pub(crate) fn from_io<T>(
        io: &T,
        on_connect_ext: Option<&ConnectCallback<T>>,
    ) -> Self {
        let ext = on_connect_ext.map(|handler| {
            let mut extensions = Extensions::new();
            handler(io, &mut extensions);
            extensions
        });

        Self(ext)
    }

    // merge self to given request's head extension.
    #[inline]

Review comment (refactor):
use doccomments in this file

```suggestion
/// Container for data that extract with ConnectCallback.
```

Code after:
    pub use http::header::{HeaderName, HeaderValue};
    pub use http::uri::PathAndQuery;
    pub use http::{uri, Error, Uri};
    pub use http::{Method, StatusCode, Version};

    pub use crate::cookie::{Cookie, CookieBuilder};
    pub use crate::header::HeaderMap;

    /// Various http headers
    pub mod header {
        pub use crate::header::*;
    }
    pub use crate::header::ContentEncoding;
    pub use crate::message::ConnectionType;
}

/// Http protocol
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum Protocol {
    Http1,
    Http2,
}

type ConnectCallback<IO> = dyn Fn(&IO, &mut Extensions);

/// Container for data that extract with ConnectCallback.
pub(crate) struct OnConnectData(Option<Extensions>);

impl Default for OnConnectData {
    fn default() -> Self {
        Self(None)
    }
}

impl OnConnectData {
    // construct self from io.
    pub(crate) fn from_io<T>(
        io: &T,
        on_connect_ext: Option<&ConnectCallback<T>>,
    ) -> Self {
        let ext = on_connect_ext.map(|handler| {
            let mut extensions = Extensions::new();
            handler(io, &mut extensions);
            extensions
        });

        Self(ext)
    }

    // merge self to given request's head extension.
    #[inline]
Repository: actix/actix-web
File: actix-http/src/lib.rs (Rust)

Code before:
pub(crate) struct OnConnectData(Option<Extensions>);

impl Default for OnConnectData {
    fn default() -> Self {
        Self(None)
    }
}

impl OnConnectData {
    // construct self from io.
    pub(crate) fn from_io<T>(
        io: &T,
        on_connect_ext: Option<&ConnectCallback<T>>,
    ) -> Self {
        let ext = on_connect_ext.map(|handler| {
            let mut extensions = Extensions::new();
            handler(io, &mut extensions);
            extensions
        });

        Self(ext)
    }

    // merge self to given request's head extension.
    #[inline]
    pub(crate) fn merge(&mut self, req: &mut Request) {
        if let Some(ref mut ext) = self.0 {
            req.head.extensions.get_mut().drain_from(ext);
        }
    }
}


Review comment (suggestion):
just to make the merge direction clear when reading the dispatcher: `merge_into`, `drain_into`, `drain_into_request`, etc.

```suggestion
    pub(crate) fn merge_into(&mut self, req: &mut Request) {
```

Code after:
pub(crate) struct OnConnectData(Option<Extensions>);

impl Default for OnConnectData {
    fn default() -> Self {
        Self(None)
    }
}

impl OnConnectData {
    // construct self from io.
    pub(crate) fn from_io<T>(
        io: &T,
        on_connect_ext: Option<&ConnectCallback<T>>,
    ) -> Self {
        let ext = on_connect_ext.map(|handler| {
            let mut extensions = Extensions::new();
            handler(io, &mut extensions);
            extensions
        });

        Self(ext)
    }

    // merge self to given request's head extension.
    #[inline]
    pub(crate) fn merge_into(&mut self, req: &mut Request) {
        if let Some(ref mut ext) = self.0 {
            req.head.extensions.get_mut().drain_from(ext);
        }
    }
}
Repository: actix/actix-web
File: actix-http/src/h1/encoder.rs (Rust)

Code before:
                        remaining -= len;
                    }
                }
            }
        });

        // final cursor synchronization with the bytes container
        //
        // SAFETY: all the bytes written up to position "pos" are initialized
        // the written byte count and pointer advancement are kept in sync
        unsafe {
            dst.advance_mut(pos);
        }

        // optimized date header, set_date writes \r\n
        if !has_date {
            config.set_date(dst);
        } else {
            // msg eof
            dst.extend_from_slice(b"\r\n");
        }

        Ok(())
    }

    #[inline]
    fn write_headers<F>(&mut self, mut f: F)
    where
        F: FnMut(&HeaderName, &Value),
    {
        match self.extra_headers() {
            Some(headers) => {
                // merging headers from head and extra headers.
                self.headers()
                    .inner
                    .iter()
                    .filter(|(name, _)| !headers.contains_key(*name))
                    .chain(headers.inner.iter())
                    .for_each(|(k, v)| f(k, v))
            }
            None => self.headers().inner.iter().for_each(|(k, v)| f(k, v)),
        }
    }
}

impl MessageType for Response<()> {
    fn status(&self) -> Option<StatusCode> {
        Some(self.head().status)
    }

    fn chunked(&self) -> bool {

Review comment (suggestion):
inlining happens inside crate boundaries by default (at compilers discretion), so either remove or `#[inline(always)]` if you think it's appropirate

Code after:
                        remaining -= len;
                    }
                }
            }
        });

        // final cursor synchronization with the bytes container
        //
        // SAFETY: all the bytes written up to position "pos" are initialized
        // the written byte count and pointer advancement are kept in sync
        unsafe {
            dst.advance_mut(pos);
        }

        // optimized date header, set_date writes \r\n
        if !has_date {
            config.set_date(dst);
        } else {
            // msg eof
            dst.extend_from_slice(b"\r\n");
        }

        Ok(())
    }

    fn write_headers<F>(&mut self, mut f: F)
    where
        F: FnMut(&HeaderName, &Value),
    {
        match self.extra_headers() {
            Some(headers) => {
                // merging headers from head and extra headers.
                self.headers()
                    .inner
                    .iter()
                    .filter(|(name, _)| !headers.contains_key(*name))
                    .chain(headers.inner.iter())
                    .for_each(|(k, v)| f(k, v))
            }
            None => self.headers().inner.iter().for_each(|(k, v)| f(k, v)),
        }
    }
}

impl MessageType for Response<()> {
    fn status(&self) -> Option<StatusCode> {
        Some(self.head().status)
    }

    fn chunked(&self) -> bool {
        self.head().chunked()
Repository: actix/actix-web
File: actix-files/src/files.rs (Rust)

Code before:
        config.register_service(rdef, guards, self, None)
    }
}

impl ServiceFactory<ServiceRequest> for Files {
    type Response = ServiceResponse;
    type Error = Error;
    type Config = ();
    type Service = FilesService;
    type InitError = ();
    type Future = LocalBoxFuture<'static, Result<Self::Service, Self::InitError>>;

    fn new_service(&self, _: ()) -> Self::Future {
        let mut srv = FilesService {
            directory: self.directory.clone(),
            index: self.index.clone(),
            show_index: self.show_index,
            redirect_to_slash: self.redirect_to_slash,
            default: None,
            renderer: self.renderer.clone(),
            mime_override: self.mime_override.clone(),
            file_flags: self.file_flags,
            guards: self.use_guards.clone(),
            hidden_files: self.hidden_files,
        };

        if let Some(ref default) = *self.default.borrow() {
            default
                .new_service(())
                .map(move |result| match result {
                    Ok(default) => {
                        srv.default = Some(default);
                        Ok(srv)
                    }
                    Err(_) => Err(()),
                })
                .boxed_local()
        } else {
            ok(srv).boxed_local()
        }
    }
}


Review comment (suggestion):
```suggestion
        let guards = self.guards.take();
```

Code after:
    pub fn default_handler<F, U>(mut self, f: F) -> Self
    where
        F: IntoServiceFactory<U, ServiceRequest>,
        U: ServiceFactory<
                ServiceRequest,
                Config = (),
                Response = ServiceResponse,
                Error = Error,
            > + 'static,
    {
        // create and configure default resource
        self.default = Rc::new(RefCell::new(Some(Rc::new(boxed::factory(
            f.into_factory().map_init_err(|_| ()),
        )))));

        self
    }

    /// Enables serving hidden files and directories, allowing a leading dots in url fragments.
    #[inline]
    pub fn use_hidden_files(mut self) -> Self {
        self.hidden_files = true;
        self
    }
}

impl HttpServiceFactory for Files {
    fn register(mut self, config: &mut AppService) {
        let guards = if self.guards.is_empty() {
            None
        } else {
            let guards = std::mem::take(&mut self.guards);
            Some(
                guards
                    .into_iter()
                    .map(|guard| -> Box<dyn Guard> { guard })
                    .collect::<Vec<_>>(),
            )
        };

        if self.default.borrow().is_none() {
            *self.default.borrow_mut() = Some(config.default_service());
        }

        let rdef = if config.is_root() {
            ResourceDef::root_prefix(&self.path)
        } else {
            ResourceDef::prefix(&self.path)
        };

        config.register_service(rdef, guards, self, None)