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

Code before:

extern crate proc_macro;
extern crate proc_macro2;

mod internals;

use std::str::FromStr;
use proc_macro::TokenStream;
use syn::DeriveInput;

#[macro_use]
mod bound;
#[macro_use]
mod fragment;

mod de;
mod pretend;
mod ser;
mod try;

#[proc_macro_derive(Serialize, attributes(serde))]
pub fn derive_serialize(input: TokenStream) -> TokenStream {
    let input: DeriveInput = syn::parse(input).unwrap();
    match ser::expand_derive_serialize(&input) {
        Ok(expanded) => expanded.into(),
        Err(msg) => TokenStream::from_str(&format!("compile_error!({:?});", msg)).unwrap(),
    }
}

#[proc_macro_derive(Deserialize, attributes(serde))]
pub fn derive_deserialize(input: TokenStream) -> TokenStream {
    let input: DeriveInput = syn::parse(input).unwrap();
    match de::expand_derive_deserialize(&input) {
        Ok(expanded) => expanded.into(),
        Err(msg) => TokenStream::from_str(&format!("compile_error!({:?});", msg)).unwrap(),
    }
}


Review comment (suggestion):
Typically macros try to avoid working with Rust source code as strings -- strings are unfriendly to our text editor or IDE when it comes to brace matching, syntax highlighting, indentation, etc. Serde's derive currently uses the [`quote`](https://github.com/dtolnay/quote) crate instead of strings of source code.

Here it's just one line so there may not be an appreciable advantage, but I think it would be better to be consistent with the code generation done in the rest of serde_derive and use `quote!` here as well.

Code after:

extern crate proc_macro;
extern crate proc_macro2;

mod internals;

use std::str::FromStr;
use proc_macro::TokenStream;
use syn::DeriveInput;

#[macro_use]
mod bound;
#[macro_use]
mod fragment;

mod de;
mod pretend;
mod ser;
mod try;

#[proc_macro_derive(Serialize, attributes(serde))]
pub fn derive_serialize(input: TokenStream) -> TokenStream {
    let input: DeriveInput = syn::parse(input).unwrap();
    match ser::expand_derive_serialize(&input) {
        Ok(expanded) => expanded.into(),
        Err(msg) => quote! {compile_error!(#msg);}.into(),
    }
}

#[proc_macro_derive(Deserialize, attributes(serde))]
pub fn derive_deserialize(input: TokenStream) -> TokenStream {
    let input: DeriveInput = syn::parse(input).unwrap();
    match de::expand_derive_deserialize(&input) {
        Ok(expanded) => expanded.into(),
        Err(msg) => quote! {compile_error!(#msg);}.into(),
    }
}
Repository: serde-rs/serde
File: serde_derive/src/internals/ast.rs (Rust)

Code before:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! A serde ast, parsed from the syn ast and ready for codegen.

use internals::attr;
use internals::check;
use internals::{Ctxt, Derive};
use syn;
use syn::punctuated::Punctuated;

/// A source data structure annotated with `#[derive(Derialize)]` and/or `#[derive(Deserialize)]`,
/// parsed into an internal representation.
pub struct Container<'a> {
    /// The struct or enum name (without generics).
    pub ident: syn::Ident,
    /// Attributes on the structure, parsed for serde.
    pub attrs: attr::Container,
    /// The contents of the struct or enum.
    pub data: Data<'a>,
    /// Any generics on the struct or enum.
    pub generics: &'a syn::Generics,
}

/// The fields of a struct or enum.
///
/// Analagous to `syn::Data`.
pub enum Data<'a> {
    Enum(Vec<Variant<'a>>),

Review comment (suggestion):
We have moved away from using the word "codegen" to describe procedural macros because it is unnecessarily intimidating. Please find a different way to phrase this without "codegen".

Code after:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! A Serde ast, parsed from the Syn ast and ready to generate Rust code.

use internals::attr;
use internals::check;
use internals::{Ctxt, Derive};
use syn;
use syn::punctuated::Punctuated;

/// A source data structure annotated with `#[derive(Serialize)]` and/or `#[derive(Deserialize)]`,
/// parsed into an internal representation.
pub struct Container<'a> {
    /// The struct or enum name (without generics).
    pub ident: syn::Ident,
    /// Attributes on the structure, parsed for Serde.
    pub attrs: attr::Container,
    /// The contents of the struct or enum.
    pub data: Data<'a>,
    /// Any generics on the struct or enum.
    pub generics: &'a syn::Generics,
}

/// The fields of a struct or enum.
///
/// Analagous to `syn::Data`.
pub enum Data<'a> {
    Enum(Vec<Variant<'a>>),
Repository: serde-rs/serde
File: serde_derive/src/internals/case.rs (Rust)

Code before:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Stuff to handle where the case of the source (e.g. `my-field`) is different to the
//! field/variant (e.g. `my_field`).

// See https://users.rust-lang.org/t/psa-dealing-with-warning-unused-import-std-ascii-asciiext-in-today-s-nightly/13726
#[allow(deprecated, unused_imports)]
use std::ascii::AsciiExt;

use std::str::FromStr;

use self::RenameRule::*;

/// The different possible ways to change case of fields in a struct, or variants in an enum.
#[derive(PartialEq)]
pub enum RenameRule {
    /// Don't apply a default rename rule.
    None,
    /// Rename direct children to "lowercase" style.
    LowerCase,
    /// Rename direct children to "UPPERCASE" style.
    UPPERCASE,
    /// Rename direct children to "PascalCase" style, as typically used for
    /// enum variants.
    PascalCase,
    /// Rename direct children to "camelCase" style.
    CamelCase,
    /// Rename direct children to "snake_case" style, as commonly used for

Review comment (suggestion):
Please change "different to" to "different from", which also works in American English.
https://en.oxforddictionaries.com/usage/different-from-than-or-to

Code after:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Code to convert the Rust-styled field/variant (e.g. `my_field`, `MyType`) to the
//! case of the source (e.g. `my-field`, `MY_FIELD`).

// See https://users.rust-lang.org/t/psa-dealing-with-warning-unused-import-std-ascii-asciiext-in-today-s-nightly/13726
#[allow(deprecated, unused_imports)]
use std::ascii::AsciiExt;

use std::str::FromStr;

use self::RenameRule::*;

/// The different possible ways to change case of fields in a struct, or variants in an enum.
#[derive(PartialEq)]
pub enum RenameRule {
    /// Don't apply a default rename rule.
    None,
    /// Rename direct children to "lowercase" style.
    LowerCase,
    /// Rename direct children to "UPPERCASE" style.
    UPPERCASE,
    /// Rename direct children to "PascalCase" style, as typically used for
    /// enum variants.
    PascalCase,
    /// Rename direct children to "camelCase" style.
    CamelCase,
    /// Rename direct children to "snake_case" style, as commonly used for
Repository: serde-rs/serde
File: serde_derive/src/internals/case.rs (Rust)

Code before:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Utility code to assist converting the case of the source (e.g. `my-field`, `MY_FIELD`) to the
//! rust-styled field/variant (e.g. `my_field`, `MyType`).

// See https://users.rust-lang.org/t/psa-dealing-with-warning-unused-import-std-ascii-asciiext-in-today-s-nightly/13726
#[allow(deprecated, unused_imports)]
use std::ascii::AsciiExt;

use std::str::FromStr;

use self::RenameRule::*;

/// The different possible ways to change case of fields in a struct, or variants in an enum.
#[derive(PartialEq)]
pub enum RenameRule {
    /// Don't apply a default rename rule.
    None,
    /// Rename direct children to "lowercase" style.
    LowerCase,
    /// Rename direct children to "UPPERCASE" style.
    UPPERCASE,
    /// Rename direct children to "PascalCase" style, as typically used for
    /// enum variants.
    PascalCase,
    /// Rename direct children to "camelCase" style.
    CamelCase,
    /// Rename direct children to "snake_case" style, as commonly used for

Review comment (refactor):
What distinction is this making between utility code and ordinary code? I would recommend avoiding the word utility in naming or documenting anything. [https://blog.golang.org/package-names#TOC_5.](https://blog.golang.org/package-names#TOC_5.) is relevant.

Code after:
// Copyright 2017 Serde Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Code to convert the Rust-styled field/variant (e.g. `my_field`, `MyType`) to the
//! case of the source (e.g. `my-field`, `MY_FIELD`).

// See https://users.rust-lang.org/t/psa-dealing-with-warning-unused-import-std-ascii-asciiext-in-today-s-nightly/13726
#[allow(deprecated, unused_imports)]
use std::ascii::AsciiExt;

use std::str::FromStr;

use self::RenameRule::*;

/// The different possible ways to change case of fields in a struct, or variants in an enum.
#[derive(PartialEq)]
pub enum RenameRule {
    /// Don't apply a default rename rule.
    None,
    /// Rename direct children to "lowercase" style.
    LowerCase,
    /// Rename direct children to "UPPERCASE" style.
    UPPERCASE,
    /// Rename direct children to "PascalCase" style, as typically used for
    /// enum variants.
    PascalCase,
    /// Rename direct children to "camelCase" style.
    CamelCase,
    /// Rename direct children to "snake_case" style, as commonly used for
Repository: serde-rs/serde
File: serde/src/private/de.rs (Rust)

Code before:
        where
            V: MapAccess<'de>,
        {
            ContentVisitor::new()
                .visit_map(visitor)
                .map(TagOrContent::Content)
        }

        fn visit_enum<V>(self, visitor: V) -> Result<Self::Value, V::Error>
        where
            V: EnumAccess<'de>,
        {
            ContentVisitor::new()
                .visit_enum(visitor)
                .map(TagOrContent::Content)
        }
    }

    /// Used by generated code to deserialize an internally tagged enum.
    ///
    /// Not public API.
    pub struct TaggedContentVisitor<T> {
        tag_name: &'static str,
        expecting: &'static str,
        value: PhantomData<T>,
    }

    impl<T> TaggedContentVisitor<T> {
        /// Visitor for the content of an internally tagged enum with the given
        /// tag name.
        pub fn new(name: &'static str, expecting: &'static str) -> Self {
            TaggedContentVisitor {
                tag_name: name,
                expecting: expecting,
                value: PhantomData,
            }
        }
    }

    impl<'de, T> Visitor<'de> for TaggedContentVisitor<T>
    where
        T: Deserialize<'de>,
    {
        type Value = (T, Content<'de>);

        fn expecting(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
            fmt.write_str(self.expecting)
        }

        fn visit_seq<S>(self, mut seq: S) -> Result<Self::Value, S::Error>
        where

Review comment (suggestion):
I'm not really convinced that this is better than the status quo, so I'd prefer to just leave it as-is.

Code after:
        where
            V: MapAccess<'de>,
        {
            ContentVisitor::new()
                .visit_map(visitor)
                .map(TagOrContent::Content)
        }

        fn visit_enum<V>(self, visitor: V) -> Result<Self::Value, V::Error>
        where
            V: EnumAccess<'de>,
        {
            ContentVisitor::new()
                .visit_enum(visitor)
                .map(TagOrContent::Content)
        }
    }

    /// Used by generated code to deserialize an internally tagged enum.
    ///
    /// Not public API.
    pub struct TaggedContentVisitor<T> {
        tag_name: &'static str,
        expecting: &'static str,
        value: PhantomData<T>,
    }

    impl<T> TaggedContentVisitor<T> {
        /// Visitor for the content of an internally tagged enum with the given
        /// tag name.
        pub fn new(name: &'static str, expecting: &'static str) -> Self {
            TaggedContentVisitor {
                tag_name: name,
                expecting,
                value: PhantomData,
            }
        }
    }

    impl<'de, T> Visitor<'de> for TaggedContentVisitor<T>
    where
        T: Deserialize<'de>,
    {
        type Value = (T, Content<'de>);

        fn expecting(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
            fmt.write_str(self.expecting)
        }

        fn visit_seq<S>(self, mut seq: S) -> Result<Self::Value, S::Error>
        where
Repository: serde-rs/serde
File: serde_derive/src/de.rs (Rust)

Code before:

            let block = Match(deserialize_internally_tagged_variant(
                params,
                variant,
                cattrs,
                quote!(__deserializer),
            ));

            quote! {
                __Field::#variant_name => #block
            }
        });

    let expecting = format!("internally tagged enum {}", params.type_name());
    let expecting = cattrs.expecting().unwrap_or(&expecting);

    quote_block! {
        #variant_visitor

        #variants_stmt

        let __tagged = try!(_serde::Deserializer::deserialize_any(
            __deserializer,
            _serde::__private::de::TaggedContentVisitor::<__Field>::new(#tag, #expecting)));
        let __deserializer = _serde::__private::de::ContentDeserializer::<__D::Error>::new(__tagged.content);

        match __tagged.tag {
            #(#variant_arms)*
        }
    }
}

fn deserialize_adjacently_tagged_enum(
    params: &Parameters,
    variants: &[Variant],
    cattrs: &attr::Container,
    tag: &str,
    content: &str,
) -> Fragment {
    let this_type = &params.this_type;
    let this_value = &params.this_value;
    let (de_impl_generics, de_ty_generics, ty_generics, where_clause) =
        split_with_de_lifetime(params);
    let delife = params.borrowed.de_lifetime();

    let (variants_stmt, variant_visitor) = prepare_enum_variant_enum(variants, cattrs);

    let variant_arms: &Vec<_> = &variants
        .iter()
        .enumerate()
        .filter(|&(_, variant)| !variant.attrs.skip_deserializing())

Review comment (suggestion):
took me a moment, but this is only ever moved into one arm, but all arms move out of it. So this makes sense

Code after:
            },
        )
    }
}

fn deserialize_internally_tagged_enum(
    params: &Parameters,
    variants: &[Variant],
    cattrs: &attr::Container,
    tag: &str,
) -> Fragment {
    let (variants_stmt, variant_visitor) = prepare_enum_variant_enum(variants, cattrs);

    // Match arms to extract a variant from a string
    let variant_arms = variants
        .iter()
        .enumerate()
        .filter(|&(_, variant)| !variant.attrs.skip_deserializing())
        .map(|(i, variant)| {
            let variant_name = field_i(i);

            let block = Match(deserialize_internally_tagged_variant(
                params,
                variant,
                cattrs,
                quote!(__deserializer),
            ));

            quote! {
                __Field::#variant_name => #block
            }
        });

    let expecting = format!("internally tagged enum {}", params.type_name());
    let expecting = cattrs.expecting().unwrap_or(&expecting);

    quote_block! {
        #variant_visitor

        #variants_stmt

        let (__tag, __content) = try!(_serde::Deserializer::deserialize_any(
            __deserializer,
            _serde::__private::de::TaggedContentVisitor::<__Field>::new(#tag, #expecting)));
        let __deserializer = _serde::__private::de::ContentDeserializer::<__D::Error>::new(__content);

        match __tag {
            #(#variant_arms)*
        }
    }
}
Repository: serde-rs/serde
File: test_suite/tests/test_annotations.rs (Rust)

Code before:
#[test]
fn test_internally_tagged_enum_new_type_with_unit() {
    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    #[serde(tag = "t")]
    enum Data {
        A(()),
    }

    assert_tokens(
        &Data::A(()),
        &[
            Token::Map { len: Some(1) },
            Token::Str("t"),
            Token::Str("A"),
            Token::MapEnd,
        ],
    );
    assert_ser_tokens(
        &Data::A(()),
        &[
            Token::Map { len: Some(1) },
            Token::Str("t"),
            Token::Str("A"),
            Token::MapEnd,
        ],
    );
}

#[test]
fn test_adjacently_tagged_enum_containing_flatten() {
    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    #[serde(tag = "t", content = "c")]
    enum Data {
        A {
            a: i32,
            #[serde(flatten)]
            flat: Flat,
        },
    }

    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    struct Flat {
        b: i32,
    }

    let data = Data::A {
        a: 0,
        flat: Flat { b: 0 },
    };

    assert_tokens(

Review comment (suggestion):
This same input is already tested by `assert_tokens` above.

```suggestion
```

Code after:
#[test]
fn test_internally_tagged_enum_new_type_with_unit() {
    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    #[serde(tag = "t")]
    enum Data {
        A(()),
    }

    assert_tokens(
        &Data::A(()),
        &[
            Token::Map { len: Some(1) },
            Token::Str("t"),
            Token::Str("A"),
            Token::MapEnd,
        ],
    );
}

#[test]
fn test_adjacently_tagged_enum_containing_flatten() {
    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    #[serde(tag = "t", content = "c")]
    enum Data {
        A {
            a: i32,
            #[serde(flatten)]
            flat: Flat,
        },
    }

    #[derive(Serialize, Deserialize, PartialEq, Debug)]
    struct Flat {
        b: i32,
    }

    let data = Data::A {
        a: 0,
        flat: Flat { b: 0 },
    };

    assert_tokens(
        &data,
        &[
            Token::Struct {
                name: "Data",
                len: 2,
            },
            Token::Str("t"),
            Token::Str("A"),
            Token::Str("c"),
Repository: serde-rs/serde
File: serde/src/de/format.rs (Rust)

Code before:
use lib::fmt::{self, Write};
use lib::str;

pub struct Buf<'a> {
    bytes: &'a mut [u8],
    offset: usize,
}

impl<'a> Buf<'a> {
    pub fn new(bytes: &'a mut [u8]) -> Self {
        Buf { bytes, offset: 0 }
    }

    pub fn as_str(&self) -> &str {
        let slice = &self.bytes[..self.offset];
        unsafe { str::from_utf8_unchecked(slice) }
    }
}

impl<'a> Write for Buf<'a> {
    fn write_str(&mut self, s: &str) -> fmt::Result {
        if self.offset + s.len() > self.bytes.len() {
            Err(fmt::Error)
        } else {
            self.bytes[self.offset..self.offset + s.len()].copy_from_slice(s.as_bytes());
            self.offset += s.len();
            Ok(())
        }
    }
}


Review comment (suggestion):
> - It changes `Buf::as_str()` to be unsafe, since it relies on the caller to only write UTF-8 into the buffer.

The API enforces that the caller only wrote UTF-8 into the buffer.

Code after:
use lib::fmt::{self, Write};
use lib::str;

pub(super) struct Buf<'a> {
    bytes: &'a mut [u8],
    offset: usize,
}

impl<'a> Buf<'a> {
    pub(super) fn new(bytes: &'a mut [u8]) -> Self {
        Buf { bytes, offset: 0 }
    }

    pub(super) unsafe fn as_str(&self) -> &str {
        let slice = &self.bytes[..self.offset];
        str::from_utf8_unchecked(slice)
    }
}

impl<'a> Write for Buf<'a> {
    fn write_str(&mut self, s: &str) -> fmt::Result {
        if self.offset + s.len() > self.bytes.len() {
            Err(fmt::Error)
        } else {
            self.bytes[self.offset..self.offset + s.len()].copy_from_slice(s.as_bytes());
            self.offset += s.len();
            Ok(())
        }
    }
}
Repository: serde-rs/serde
File: serde/src/private/size_hint.rs (Rust)

Code before:
use lib::*;

pub fn from_bounds<I>(iter: &I) -> Option<usize>
where
    I: Iterator,
{
    helper(iter.size_hint())
}

#[cfg(any(feature = "std", feature = "alloc"))]
#[inline]
pub fn cautious(hint: Option<usize>) -> usize {
    cmp::min(hint.unwrap_or(0), 4096)
}

fn helper(bounds: (usize, Option<usize>)) -> Option<usize> {
    match bounds {
        (lower, Some(upper)) if lower == upper => Some(upper),
        _ => None,
    }
}


Review comment (performance):
It would be good to cache the result of `mem::size_of::<Element>()` and reuse it at line 19 below

Code after:
use lib::*;

pub fn from_bounds<I>(iter: &I) -> Option<usize>
where
    I: Iterator,
{
    helper(iter.size_hint())
}

#[cfg(any(feature = "std", feature = "alloc"))]
pub fn cautious<Element>(hint: Option<usize>) -> usize {
    const MAX_PREALLOC_BYTES: usize = 1024 * 1024;

    if mem::size_of::<Element>() == 0 {
        0
    } else {
        cmp::min(
            hint.unwrap_or(0),
            MAX_PREALLOC_BYTES / mem::size_of::<Element>(),
        )
    }
}

fn helper(bounds: (usize, Option<usize>)) -> Option<usize> {
    match bounds {
        (lower, Some(upper)) if lower == upper => Some(upper),
        _ => None,
    }
}