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

Code before:
mod uri;
mod with;

pub mod client;
pub mod error;
pub mod fs;
pub mod middleware;
pub mod multipart;
pub mod pred;
pub mod server;
pub mod test;
pub mod ws;
pub use application::App;
pub use body::{Binary, Body};
pub use context::HttpContext;
pub use error::{Error, ResponseError, Result};
pub use extractor::{Form, Path, Query};
pub use handler::{
    AsyncResponder, Either, FromRequest, FutureResponse, Responder, State,
};
pub use httpmessage::HttpMessage;
pub use httprequest::HttpRequest;
pub use httpresponse::HttpResponse;
pub use json::Json;
pub use scope::Scope;
pub use actix;

#[doc(hidden)]
#[deprecated(since = "0.6.2", note = "please use `use actix_web::ws::WsWriter`")]
pub use ws::WsWriter;

#[cfg(feature = "openssl")]
pub(crate) const HAS_OPENSSL: bool = true;
#[cfg(not(feature = "openssl"))]
pub(crate) const HAS_OPENSSL: bool = false;

#[cfg(feature = "tls")]
pub(crate) const HAS_TLS: bool = true;
#[cfg(not(feature = "tls"))]
pub(crate) const HAS_TLS: bool = false;

pub mod dev {
    //! The `actix-web` prelude for library developers
    //!
    //! The purpose of this module is to alleviate imports of many common actix
    //! traits by adding a glob import to the top of actix heavy modules:
    //!
    //! ```
    //! # #![allow(unused_imports)]
    //! use actix_web::dev::*;
    //! ```

Review comment (suggestion):
I think, this should be 

```rust
pub mod actix {
   pub use actix::prelude::*;
}
```

Code after:
mod uri;
mod with;

pub mod client;
pub mod error;
pub mod fs;
pub mod middleware;
pub mod multipart;
pub mod pred;
pub mod server;
pub mod test;
pub mod ws;
pub use application::App;
pub use body::{Binary, Body};
pub use context::HttpContext;
pub use error::{Error, ResponseError, Result};
pub use extractor::{Form, Path, Query};
pub use handler::{
    AsyncResponder, Either, FromRequest, FutureResponse, Responder, State,
};
pub use httpmessage::HttpMessage;
pub use httprequest::HttpRequest;
pub use httpresponse::HttpResponse;
pub use json::Json;
pub use scope::Scope;

pub mod actix {
    //! Re-exports [actix's](https://docs.rs/actix) prelude

    pub use actix_inner::prelude::*;
}

#[doc(hidden)]
#[deprecated(since = "0.6.2", note = "please use `use actix_web::ws::WsWriter`")]
pub use ws::WsWriter;

#[cfg(feature = "openssl")]
pub(crate) const HAS_OPENSSL: bool = true;
#[cfg(not(feature = "openssl"))]
pub(crate) const HAS_OPENSSL: bool = false;

#[cfg(feature = "tls")]
pub(crate) const HAS_TLS: bool = true;
#[cfg(not(feature = "tls"))]
pub(crate) const HAS_TLS: bool = false;

pub mod dev {
    //! The `actix-web` prelude for library developers
    //!
    //! The purpose of this module is to alleviate imports of many common actix
    //! traits by adding a glob import to the top of actix heavy modules:
Repository: actix/actix-web
File: src/httpcodes.rs (Rust)

Code before:
//! Basic http responses
#![allow(non_upper_case_globals)]
use http::StatusCode;
use httpresponse::{HttpResponse, HttpResponseBuilder};

macro_rules! STATIC_RESP {
    ($name:ident, $status:expr) => {
        #[allow(non_snake_case)]
        pub fn $name() -> HttpResponseBuilder {
            HttpResponse::build($status)
        }
    };
}

impl HttpResponse {
    STATIC_RESP!(Ok, StatusCode::OK);
    STATIC_RESP!(Created, StatusCode::CREATED);
    STATIC_RESP!(Accepted, StatusCode::ACCEPTED);
    STATIC_RESP!(
        NonAuthoritativeInformation,
        StatusCode::NON_AUTHORITATIVE_INFORMATION
    );

    STATIC_RESP!(NoContent, StatusCode::NO_CONTENT);
    STATIC_RESP!(ResetContent, StatusCode::RESET_CONTENT);
    STATIC_RESP!(PartialContent, StatusCode::PARTIAL_CONTENT);
    STATIC_RESP!(MultiStatus, StatusCode::MULTI_STATUS);
    STATIC_RESP!(AlreadyReported, StatusCode::ALREADY_REPORTED);

    STATIC_RESP!(MultipleChoices, StatusCode::MULTIPLE_CHOICES);
    STATIC_RESP!(MovedPermanenty, StatusCode::MOVED_PERMANENTLY);
    STATIC_RESP!(MovedPermanently, StatusCode::MOVED_PERMANENTLY);
    STATIC_RESP!(Found, StatusCode::FOUND);

Review comment (suggestion):
macros are evil but you could play with transforming the `$name:ident` to a `$name:expr` (via `$name:tt`) and putting it in a `#[doc = $name]` attr

Code after:
//! Basic http responses
#![allow(non_upper_case_globals)]
use http::StatusCode;
use httpresponse::{HttpResponse, HttpResponseBuilder};

macro_rules! STATIC_RESP {
    ($name:ident, $status:expr) => {
        #[allow(non_snake_case, missing_docs)]
        pub fn $name() -> HttpResponseBuilder {
            HttpResponse::build($status)
        }
    };
}

impl HttpResponse {
    STATIC_RESP!(Ok, StatusCode::OK);
    STATIC_RESP!(Created, StatusCode::CREATED);
    STATIC_RESP!(Accepted, StatusCode::ACCEPTED);
    STATIC_RESP!(
        NonAuthoritativeInformation,
        StatusCode::NON_AUTHORITATIVE_INFORMATION
    );

    STATIC_RESP!(NoContent, StatusCode::NO_CONTENT);
    STATIC_RESP!(ResetContent, StatusCode::RESET_CONTENT);
    STATIC_RESP!(PartialContent, StatusCode::PARTIAL_CONTENT);
    STATIC_RESP!(MultiStatus, StatusCode::MULTI_STATUS);
    STATIC_RESP!(AlreadyReported, StatusCode::ALREADY_REPORTED);

    STATIC_RESP!(MultipleChoices, StatusCode::MULTIPLE_CHOICES);
    STATIC_RESP!(MovedPermanenty, StatusCode::MOVED_PERMANENTLY);
    STATIC_RESP!(MovedPermanently, StatusCode::MOVED_PERMANENTLY);
    STATIC_RESP!(Found, StatusCode::FOUND);
Repository: actix/actix-web
File: src/server/h1writer.rs (Rust)

Code before:

    fn write_eof(&mut self) -> io::Result<WriterState> {
        if !self.encoder.write_eof()? {
            Err(io::Error::new(
                io::ErrorKind::Other,
                "Last payload item, but eof is not reached",
            ))
        } else if self.buffer.len() > MAX_WRITE_BUFFER_SIZE {
            Ok(WriterState::Pause)
        } else {
            Ok(WriterState::Done)
        }
    }

    #[inline]
    fn poll_completed(&mut self, shutdown: bool) -> Poll<(), io::Error> {
        if !self.buffer.is_empty() {
            let buf: &[u8] =
                unsafe { &mut *(self.buffer.as_ref() as *const _ as *mut _) };
            let written = self.write_data(buf)?;
            let _ = self.buffer.split_to(written);
            if shutdown && !self.buffer.is_empty()
                || (self.buffer.len() > self.buffer_capacity)
            {
                return Ok(Async::NotReady);
            }
        }
        if shutdown {
            self.stream.shutdown()
        } else {
            Ok(Async::Ready(()))
        }
    }
}


Review comment (suggestion):
It seems this change introduce some code duplication...

Technically getting `&self.buffer` is not a problem as we ain't going to access it in `write_data` and I guess that was the reason for this unsafe i.e. remove code duplication.

Code after:
        }

        if self.buffer.len() > self.buffer_capacity {
            Ok(WriterState::Pause)
        } else {
            Ok(WriterState::Done)
        }
    }

    fn write_eof(&mut self) -> io::Result<WriterState> {
        if !self.encoder.write_eof()? {
            Err(io::Error::new(
                io::ErrorKind::Other,
                "Last payload item, but eof is not reached",
            ))
        } else if self.buffer.len() > MAX_WRITE_BUFFER_SIZE {
            Ok(WriterState::Pause)
        } else {
            Ok(WriterState::Done)
        }
    }

    #[inline]
    fn poll_completed(&mut self, shutdown: bool) -> Poll<(), io::Error> {
        if !self.buffer.is_empty() {
            let written = {
                match Self::write_data(&mut self.stream, self.buffer.as_ref()) {
                    Err(err) => {
                        if err.kind() == io::ErrorKind::WriteZero {
                            self.disconnected();
                        }
    
                        return Err(err);
                    }
                    Ok(val) => val,
                }
            };
            let _ = self.buffer.split_to(written);
            if shutdown && !self.buffer.is_empty()
                || (self.buffer.len() > self.buffer_capacity)
            {
                return Ok(Async::NotReady);
            }
        }
        if shutdown {
            self.stream.shutdown()
        } else {
            Ok(Async::Ready(()))
        }
    }
}
Repository: actix/actix-web
File: src/ws/context.rs (Rust)

Code before:
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, actor, mb)))
    }

    /// Create a new Websocket context
    pub fn with_factory<F>(req: HttpRequest<S>, f: F) -> Body
    where
        F: FnOnce(&mut Self) -> A + 'static,
    {
        let mb = Mailbox::default();
        let mut ctx = WebsocketContext {
            inner: ContextParts::new(mb.sender_producer()),
            stream: None,
            request: req,
            disconnected: false,
        };

        let act = f(&mut ctx);
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, act, mb)))
    }
}

impl<A, S> WebsocketContext<A, S>
where
    A: Actor<Context = Self>,
{
    /// Write payload
    #[inline]
    pub fn write(&mut self, data: Binary) {
        if !self.disconnected {
            if self.stream.is_none() {
                self.stream = Some(SmallVec::new());
            }
            let stream = self.stream.as_mut().unwrap();
            stream.push(ContextFrame::Chunk(Some(data)));
        } else {
            warn!("Trying to write to disconnected response");
        }
    }

    /// Shared application state
    #[inline]
    pub fn state(&self) -> &S {
        self.request.state()
    }

    /// Incoming request
    #[inline]
    pub fn request(&mut self) -> &mut HttpRequest<S> {
        &mut self.request
    }


Review comment (suggestion):
Please describe in documentation to this method how user should create `Binary` as user should not manually initialize it.
and instead use proper constructor.

Code after:
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, actor, mb)))
    }

    /// Create a new Websocket context
    pub fn with_factory<F>(req: HttpRequest<S>, f: F) -> Body
    where
        F: FnOnce(&mut Self) -> A + 'static,
    {
        let mb = Mailbox::default();
        let mut ctx = WebsocketContext {
            inner: ContextParts::new(mb.sender_producer()),
            stream: None,
            request: req,
            disconnected: false,
        };

        let act = f(&mut ctx);
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, act, mb)))
    }
}

impl<A, S> WebsocketContext<A, S>
where
    A: Actor<Context = Self>,
{
    /// Write payload
    ///
    /// This is a low-level function that accepts framed messages that should
    /// be created using `Frame::message()`. If you want to send text or binary
    /// data you should prefer the `text()` or `binary()` convenience functions
    /// that handle the framing for you.
    #[inline]
    pub fn write_raw(&mut self, data: FramedMessage) {
        if !self.disconnected {
            if self.stream.is_none() {
                self.stream = Some(SmallVec::new());
            }
            let stream = self.stream.as_mut().unwrap();
            stream.push(ContextFrame::Chunk(Some(data.0)));
        } else {
            warn!("Trying to write to disconnected response");
        }
    }

    /// Shared application state
    #[inline]
    pub fn state(&self) -> &S {
        self.request.state()
    }

    /// Incoming request
Repository: actix/actix-web
File: src/ws/context.rs (Rust)

Code before:
    {
        let mb = Mailbox::default();
        let mut ctx = WebsocketContext {
            inner: ContextParts::new(mb.sender_producer()),
            stream: None,
            request: req,
            disconnected: false,
        };

        let act = f(&mut ctx);
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, act, mb)))
    }
}

impl<A, S> WebsocketContext<A, S>
where
    A: Actor<Context = Self>,
{
    /// Write payload
    #[inline]
    pub fn write(&mut self, data: Binary) {
        if !self.disconnected {
            if self.stream.is_none() {
                self.stream = Some(SmallVec::new());
            }
            let stream = self.stream.as_mut().unwrap();
            stream.push(ContextFrame::Chunk(Some(data)));
        } else {
            warn!("Trying to write to disconnected response");
        }
    }

    /// Shared application state
    #[inline]
    pub fn state(&self) -> &S {
        self.request.state()
    }

    /// Incoming request
    #[inline]
    pub fn request(&mut self) -> &mut HttpRequest<S> {
        &mut self.request
    }

    /// Returns drain future
    pub fn drain(&mut self) -> Drain<A> {
        let (tx, rx) = oneshot::channel();
        self.add_frame(ContextFrame::Drain(tx));
        Drain::new(rx)
    }


Review comment (suggestion):
Do you think it may be wise for us to introduce wrapper type for websocket frame, to avoid potential misuse?(e.g. user fill Binary himself)

Code after:
    {
        let mb = Mailbox::default();
        let mut ctx = WebsocketContext {
            inner: ContextParts::new(mb.sender_producer()),
            stream: None,
            request: req,
            disconnected: false,
        };

        let act = f(&mut ctx);
        Body::Actor(Box::new(WebsocketContextFut::new(ctx, act, mb)))
    }
}

impl<A, S> WebsocketContext<A, S>
where
    A: Actor<Context = Self>,
{
    /// Write payload
    ///
    /// This is a low-level function that accepts framed messages that should
    /// be created using `Frame::message()`. If you want to send text or binary
    /// data you should prefer the `text()` or `binary()` convenience functions
    /// that handle the framing for you.
    #[inline]
    pub fn write_raw(&mut self, data: FramedMessage) {
        if !self.disconnected {
            if self.stream.is_none() {
                self.stream = Some(SmallVec::new());
            }
            let stream = self.stream.as_mut().unwrap();
            stream.push(ContextFrame::Chunk(Some(data.0)));
        } else {
            warn!("Trying to write to disconnected response");
        }
    }

    /// Shared application state
    #[inline]
    pub fn state(&self) -> &S {
        self.request.state()
    }

    /// Incoming request
    #[inline]
    pub fn request(&mut self) -> &mut HttpRequest<S> {
        &mut self.request
    }

    /// Returns drain future
    pub fn drain(&mut self) -> Drain<A> {
Repository: actix/actix-web
File: src/ws/frame.rs (Rust)

Code before:
impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// A `Binary` representing a `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedBinary(pub Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(
            &[0b0000_0001u8, 0b0000_0001u8][..],
        ).freeze())));

Review comment (suggestion):
`pub` allows user to manually create the instance, I think we should prohibit it?

User should only correctly framed messages.

Code after:
impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedMessage(pub(crate) Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(
            &[0b0000_0001u8, 0b0000_0001u8][..],
        ).freeze())));
Repository: actix/actix-web
File: src/ws/frame.rs (Rust)

Code before:
}

impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// A `Binary` representing a `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedBinary(pub Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(

Review comment (suggestion):
I think it would be better to say just:

>`WebSocket` framed message

We don't really need to overload users :)


Code after:
}

impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedMessage(pub(crate) Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(
Repository: actix/actix-web
File: src/ws/frame.rs (Rust)

Code before:
impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// A `Binary` representing a `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedBinary(pub Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(
            &[0b0000_0001u8, 0b0000_0001u8][..],
        ).freeze())));

Review comment (suggestion):
Maybe name it `FramedMessage`?

Even `write_raw` says now 😄 

>This is a low-level function that accepts framed messages

Code after:
impl fmt::Display for Frame {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "
<FRAME>
    final: {}
    opcode: {}
    payload length: {}
    payload: 0x{}
</FRAME>",
            self.finished,
            self.opcode,
            self.payload.len(),
            self.payload
                .as_ref()
                .iter()
                .map(|byte| format!("{:x}", byte))
                .collect::<String>()
        )
    }
}

/// `WebSocket` message with framing.
#[derive(Debug)]
pub struct FramedMessage(pub(crate) Binary);

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

    fn is_none(frm: &Poll<Option<Frame>, ProtocolError>) -> bool {
        match *frm {
            Ok(Async::Ready(None)) => true,
            _ => false,
        }
    }

    fn extract(frm: Poll<Option<Frame>, ProtocolError>) -> Frame {
        match frm {
            Ok(Async::Ready(Some(frame))) => frame,
            _ => unreachable!("error"),
        }
    }

    #[test]
    fn test_parse() {
        let mut buf = PayloadBuffer::new(once(Ok(BytesMut::from(
            &[0b0000_0001u8, 0b0000_0001u8][..],
        ).freeze())));