ronantakizawa/codereview-bench huggingface dataset:
Repository: google/comprehensive-rust
File: src/pattern-matching/exercise.rs (Rust)

Code before:

#[test]
fn test_sum() {
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Add,
            left: Box::new(Expression::Value(10)),
            right: Box::new(Expression::Value(20)),
        }),
        Ok(30)
    );
}

#[test]
fn test_recursion() {
    let term1 = Expression::Op {
        op: Operation::Mul,
        left: Box::new(Expression::Value(10)),
        right: Box::new(Expression::Value(9)),
    };
    let term2 = Expression::Op {
        op: Operation::Mul,
        left: Box::new(Expression::Op {
            op: Operation::Sub,
            left: Box::new(Expression::Value(3)),
            right: Box::new(Expression::Value(0)),
        }),
        right: Box::new(Expression::Value(5)),
    };
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Add,
            left: Box::new(term1),
            right: Box::new(term2),
        }),
        Ok(105)
    );
}

#[test]
fn test_error() {
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Div,
            left: Box::new(Expression::Value(99)),
            right: Box::new(Expression::Value(0)),
        }),
        Err(String::from("division by zero"))
    );
}
// ANCHOR_END: tests

Review comment (bug):
3-0 = 3, so this isn't a very interesting case of subtraction.

I guess the idea here is to uncover the bug where _any_ expression with 0 in the RHS is considered an error? I'm not convinced that's worth covering in an exercise like this, but maybe it could be done in another test case, rather than modifying this one.

Code after:

#[test]
fn test_sum() {
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Add,
            left: Box::new(Expression::Value(10)),
            right: Box::new(Expression::Value(20)),
        }),
        Ok(30)
    );
}

#[test]
fn test_recursion() {
    let term1 = Expression::Op {
        op: Operation::Mul,
        left: Box::new(Expression::Value(10)),
        right: Box::new(Expression::Value(9)),
    };
    let term2 = Expression::Op {
        op: Operation::Mul,
        left: Box::new(Expression::Op {
            op: Operation::Sub,
            left: Box::new(Expression::Value(3)),
            right: Box::new(Expression::Value(4)),
        }),
        right: Box::new(Expression::Value(5)),
    };
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Add,
            left: Box::new(term1),
            right: Box::new(term2),
        }),
        Ok(85)
    );
}

#[test]
fn test_zeros() {
    assert_eq!(
        eval(Expression::Op {
            op: Operation::Add,
            left: Box::new(Expression::Value(0)),
            right: Box::new(Expression::Value(0))
        }),
        Ok(0)
    );
    assert_eq!(
        eval(Expression::Op {
Repository: google/comprehensive-rust
File: xtask/src/main.rs (Rust)

Code before:
];

fn main() {
    if let Err(e) = execute_task() {
        eprintln!("{e}");
        std::process::exit(-1);
    }
}

fn execute_task() -> Result<(), DynError> {
    let task = env::args().nth(1);
    match task.as_deref() {
        Some("install-tools") => install_tools()?,
        _ => print_help(task.as_deref()),
    }
    Ok(())
}

fn install_tools() -> Result<(), DynError> {
    println!("Installing project tools...");

    for args in INSTALLATION_ARGS.iter() {
        Command::new(env!("CARGO"))
            .arg("install")
            .args(args.iter().filter(|a| **a != ""))
            .status()?;
    }

    Ok(())
}

fn print_help(task: Option<&str>) {
    if let Some(t) = task {
        eprintln!(
            "Unrecognized task '{t}'. Available tasks:

install-tools            Installs the tools the project depends on.

Run with `cargo xtask [task]`.
"
        );
    } else {
        eprintln!("Missing task. To execute a task run `cargo xtask [task]`.");
    }
}


Review comment (suggestion):
the `status()` result is currently not used. This provides an `ExitStatus` that can be any value, e.g. 101.
You could check this status with e.g. `.success()` and propagate this to the caller. This script is being run in the CI pipeline without being watched and the error code is important for determining the success.

Code after:
        _ => {
            return Err(Box::from(get_help_string(task.as_deref())));
        }
    }
    Ok(())
}

fn install_tools() -> Result<(), DynError> {
    println!("Installing project tools...");

    let install_args: Vec<Vec<&str>> = vec![
        // The --locked flag is important for reproducible builds. It also
        // avoids breakage due to skews between mdbook and mdbook-svgbob.
        vec!["mdbook", "--locked", "--version", "0.4.44"],
        vec!["mdbook-svgbob", "--locked", "--version", "0.2.1"],
        vec!["mdbook-pandoc", "--locked", "--version", "0.9.3"],
        vec!["mdbook-i18n-helpers", "--locked", "--version", "0.3.5"],
        vec!["i18n-report", "--locked", "--version", "0.2.0"],
        // These packages are located in this repository
        vec!["--path", "mdbook-exerciser", "--locked"],
        vec!["--path", "mdbook-course", "--locked"],
    ];

    for args in &install_args {
        let status = Command::new(env!("CARGO"))
            .arg("install")
            .args(args)
            .status()
            .expect("Failed to execute cargo install");

        if !status.success() {
            let error_message = format!(
                "Command 'cargo install {}' exited with status code: {}",
                args.join(" "),
                status.code().unwrap()
            );
            return Err(Box::from(error_message));
        }
    }

    Ok(())
}

fn get_help_string(task: Option<&str>) -> String {
    if let Some(t) = task {
        format!(
            "Unrecognized task '{t}'. Available tasks:

install-tools            Installs the tools the project depends on."
        )
    } else {
Repository: google/comprehensive-rust
File: xtask/src/main.rs (Rust)

Code before:
            .arg("install")
            .args(args)
            .status()
            .expect("Failed to execute cargo install");

        if !status.success() {
            let error_message = format!(
                "cargo install {} {} exited with status code: {}",
                args.get(0).unwrap(),
                args.get(1).unwrap(),
                status.code().unwrap()
            );
            return Err(Box::from(error_message));
        }
    }

    Ok(())
}

fn get_help_string(task: Option<&str>) -> String {
    if let Some(t) = task {
        format!(
            "Unrecognized task '{t}'. Available tasks:

install-tools            Installs the tools the project depends on.
"
        )
    } else {
        "Missing task. To execute a task run `cargo xtask [task]`.".to_string()
    }
}


Review comment (suggestion):
This variant still has a manual new line at the end that is printed with an additional new line if the error is returned.

Code after:
            .arg("install")
            .args(args)
            .status()
            .expect("Failed to execute cargo install");

        if !status.success() {
            let error_message = format!(
                "Command 'cargo install {}' exited with status code: {}",
                args.join(" "),
                status.code().unwrap()
            );
            return Err(Box::from(error_message));
        }
    }

    Ok(())
}

fn get_help_string(task: Option<&str>) -> String {
    if let Some(t) = task {
        format!(
            "Unrecognized task '{t}'. Available tasks:

install-tools            Installs the tools the project depends on."
        )
    } else {
        "Missing task. To execute a task run `cargo xtask [task]`.".to_string()
    }
}
Repository: google/flatbuffers
File: tests/cpp17/generated_cpp17/monster_test_generated.h (C/C++)

Code before:
// automatically generated by the FlatBuffers compiler, do not modify


#ifndef FLATBUFFERS_GENERATED_MONSTERTEST_MYGAME_EXAMPLE_H_
#define FLATBUFFERS_GENERATED_MONSTERTEST_MYGAME_EXAMPLE_H_

#include "flatbuffers/flatbuffers.h"
#include "flatbuffers/flexbuffers.h"

// Ensure the included flatbuffers.h is the same version as when this file was
// generated, otherwise it may not be compatible.
static_assert(FLATBUFFERS_VERSION_MAJOR == 2 &&
              FLATBUFFERS_VERSION_MINOR == 0 &&
              FLATBUFFERS_VERSION_REVISION == 6,
             "Non-compatible flatbuffers version included");

static inline bool VerifyNestedFlexBuffer(const flatbuffers::Vector<uint8_t> *nv,
                                   flatbuffers::Verifier &verifier) {
   if (!nv) return true;
   return verifier.Check(flexbuffers::VerifyBuffer(
      nv->data(), nv->size(), verifier.GetFlexReuseTracker()));
}

namespace MyGame {

struct InParentNamespace;
struct InParentNamespaceBuilder;
struct InParentNamespaceT;

namespace Example2 {

struct Monster;
struct MonsterBuilder;
struct MonsterT;

}  // namespace Example2

namespace Example {

struct Test;

struct TestSimpleTableWithEnum;

Review comment (performance):
how is this in the `flexbuffers` namespace?

What happens if two files with this function in it get included? I think that's still a compiler error.
If that is so, you can either:
* stick an `#ifdef`` around this function much like include guards
* or actually stick it in a `flexflatutils.h` or similar, a file only to be included after both are used (maybe better)
* or (for completeness), you could generate this function specific to the field name it is used for
* or (for completeness) do the body of this code entirely inline (which would call `flex()` multiple times which is slower.

the 2nd one maybe best?

Code after:
// automatically generated by the FlatBuffers compiler, do not modify


#ifndef FLATBUFFERS_GENERATED_MONSTERTEST_MYGAME_EXAMPLE_H_
#define FLATBUFFERS_GENERATED_MONSTERTEST_MYGAME_EXAMPLE_H_

#include "flatbuffers/flatbuffers.h"
#include "flatbuffers/flexbuffers.h"
#include "flatbuffers/flex_flat_util.h"

// Ensure the included flatbuffers.h is the same version as when this file was
// generated, otherwise it may not be compatible.
static_assert(FLATBUFFERS_VERSION_MAJOR == 2 &&
              FLATBUFFERS_VERSION_MINOR == 0 &&
              FLATBUFFERS_VERSION_REVISION == 6,
             "Non-compatible flatbuffers version included");

namespace MyGame {

struct InParentNamespace;
struct InParentNamespaceBuilder;
struct InParentNamespaceT;

namespace Example2 {

struct Monster;
struct MonsterBuilder;
struct MonsterT;

}  // namespace Example2

namespace Example {

struct Test;

struct TestSimpleTableWithEnum;
struct TestSimpleTableWithEnumBuilder;
struct TestSimpleTableWithEnumT;

struct Vec3;

struct Ability;
Repository: google/flatbuffers
File: tests/MyGame/Example/Stat.go (Go)

Code before:
func (rcv *Stat) Count() uint16 {
	o := flatbuffers.UOffsetT(rcv._tab.Offset(8))
	if o != 0 {
		return rcv._tab.GetUint16(o + rcv._tab.Pos)
	}
	return 0
}

func (rcv *Stat) MutateCount(n uint16) bool {
	return rcv._tab.MutateUint16Slot(8, n)
}

func StatKeyCompare(o1, o2 flatbuffers.UOffsetT, buf []byte) bool {
	val1 := flatbuffers.GetUint16(buf[flatbuffers.GetFieldOffset(buf, 8, o1):])
	val2 := flatbuffers.GetUint16(buf[flatbuffers.GetFieldOffset(buf, 8, o2):])
	return val1 < val2
}

func (rcv *Stat) LookupByKey(key uint16, vectorLocation flatbuffers.UOffsetT, buf []byte) bool {
	span := flatbuffers.GetUOffsetT(buf[vectorLocation - 4:])
	start := flatbuffers.UOffsetT(0)
	for span != 0 {
		middle := span / 2
		tableOffset := flatbuffers.GetIndirectOffset(buf, vectorLocation+ 4 * (start + middle))
		val := flatbuffers.GetUint16(buf[flatbuffers.GetFieldOffset(buf, 8, flatbuffers.UOffsetT(len(buf)) - tableOffset):])
		comp := int(val) - int(key)
		if comp > 0 {
			span = middle
		} else if comp < 0 {
			middle += 1
			start += middle
			span -= middle
		} else {
			rcv.Init(buf, tableOffset)
			return true
		}
	}
	return false
}

func StatStart(builder *flatbuffers.Builder) {
	builder.StartObject(3)
}
func StatAddId(builder *flatbuffers.Builder, id flatbuffers.UOffsetT) {
	builder.PrependUOffsetTSlot(0, flatbuffers.UOffsetT(id), 0)
}
func StatAddVal(builder *flatbuffers.Builder, val int64) {
	builder.PrependInt64Slot(1, val, 0)
}
func StatAddCount(builder *flatbuffers.Builder, count uint16) {
	builder.PrependUint16Slot(2, count, 0)

Review comment (bug):
I know we had bugs in other languages to this part, where we try to read a field value directly from the buffer instead of using the public API getters. This matters since the getters include the default (not-written to the buffer) values that won't exist in buffer.

Code after:
func (rcv *Stat) Count() uint16 {
	o := flatbuffers.UOffsetT(rcv._tab.Offset(8))
	if o != 0 {
		return rcv._tab.GetUint16(o + rcv._tab.Pos)
	}
	return 0
}

func (rcv *Stat) MutateCount(n uint16) bool {
	return rcv._tab.MutateUint16Slot(8, n)
}

func StatKeyCompare(o1, o2 flatbuffers.UOffsetT, buf []byte) bool {
	obj1 := &Stat{}
	obj2 := &Stat{}
	obj1.Init(buf, flatbuffers.UOffsetT(len(buf)) - o1)
	obj2.Init(buf, flatbuffers.UOffsetT(len(buf)) - o2)
	return obj1.Count() < obj2.Count()
}

func (rcv *Stat) LookupByKey(key uint16, vectorLocation flatbuffers.UOffsetT, buf []byte) bool {
	span := flatbuffers.GetUOffsetT(buf[vectorLocation - 4:])
	start := flatbuffers.UOffsetT(0)
	for span != 0 {
		middle := span / 2
		tableOffset := flatbuffers.GetIndirectOffset(buf, vectorLocation+ 4 * (start + middle))
		obj := &Stat{}
		obj.Init(buf, tableOffset)
		val := obj.Count()
		comp := 0
		if val > key {
			comp = 1
		} else if val < key {
			comp = -1
		}
		if comp > 0 {
			span = middle
		} else if comp < 0 {
			middle += 1
			start += middle
			span -= middle
		} else {
			rcv.Init(buf, tableOffset)
			return true
		}
	}
	return false
}

func StatStart(builder *flatbuffers.Builder) {
	builder.StartObject(3)
Repository: google/flatbuffers
File: tests/key_field/key_field_sample_generated.h (C/C++)

Code before:
  }
  Baz()
      : a_(),
        b_(0) {
  }
  Baz(uint8_t _b)
      : a_(),
        b_(flatbuffers::EndianScalar(_b)) {
  }
  Baz(flatbuffers::span<const uint8_t, 4> _a, uint8_t _b)
      : b_(flatbuffers::EndianScalar(_b)) {
    flatbuffers::CastToArray(a_).CopyFromSpan(_a);
  }
  const flatbuffers::Array<uint8_t, 4> *a() const {
    return &flatbuffers::CastToArray(a_);
  }
  flatbuffers::Array<uint8_t, 4> *mutable_a() {
    return &flatbuffers::CastToArray(a_);
  }
  bool KeyCompareLessThan(const Baz * const o) const {
    return KeyCompareWithValue(o->a()) < 0;
  }
  int KeyCompareWithValue(const flatbuffers::Array<uint8_t, 4> *_a) const {
    const auto* curr_a = a();
    for (auto i = 0; i < curr_a->size(); i++) {
      const auto a_l = curr_a->Get(i);
      const auto a_r = _a->Get(i);
      if(a_l != a_r)
        return static_cast<int>(a_l > a_r) - static_cast<int>(a_l < a_r);
    }
    return 0;
  }
  uint8_t b() const {
    return flatbuffers::EndianScalar(b_);
  }
  void mutate_b(uint8_t _b) {
    flatbuffers::WriteScalar(&b_, _b);
  }
};
FLATBUFFERS_STRUCT_END(Baz, 5);

inline bool operator==(const Baz &lhs, const Baz &rhs) {
  return
      (lhs.a() == rhs.a()) &&
      (lhs.b() == rhs.b());
}

inline bool operator!=(const Baz &lhs, const Baz &rhs) {
    return !(lhs == rhs);
}


Review comment (suggestion):
I would just name these `lhs` and `rhs` and forgo adding the "name" part. I don't think you lose much and it will be smaller code.

Code after:
  }
  Baz()
      : a_(),
        b_(0) {
  }
  Baz(uint8_t _b)
      : a_(),
        b_(flatbuffers::EndianScalar(_b)) {
  }
  Baz(flatbuffers::span<const uint8_t, 4> _a, uint8_t _b)
      : b_(flatbuffers::EndianScalar(_b)) {
    flatbuffers::CastToArray(a_).CopyFromSpan(_a);
  }
  const flatbuffers::Array<uint8_t, 4> *a() const {
    return &flatbuffers::CastToArray(a_);
  }
  flatbuffers::Array<uint8_t, 4> *mutable_a() {
    return &flatbuffers::CastToArray(a_);
  }
  bool KeyCompareLessThan(const Baz * const o) const {
    return KeyCompareWithValue(o->a()) < 0;
  }
  int KeyCompareWithValue(const flatbuffers::Array<uint8_t, 4> *_a) const {
    const flatbuffers::Array<uint8_t, 4> *curr_a = a();
    for (flatbuffers::uoffset_t i = 0; i < curr_a->size(); i++) {
      const auto lhs = curr_a->Get(i);
      const auto rhs = _a->Get(i);
      if(lhs != rhs)
        return static_cast<int>(lhs > rhs) - static_cast<int>(lhs < rhs);
    }
    return 0;
  }
  uint8_t b() const {
    return flatbuffers::EndianScalar(b_);
  }
  void mutate_b(uint8_t _b) {
    flatbuffers::WriteScalar(&b_, _b);
  }
};
FLATBUFFERS_STRUCT_END(Baz, 5);

inline bool operator==(const Baz &lhs, const Baz &rhs) {
  return
      (lhs.a() == rhs.a()) &&
      (lhs.b() == rhs.b());
}

inline bool operator!=(const Baz &lhs, const Baz &rhs) {
    return !(lhs == rhs);
}
Repository: google/flatbuffers
File: tests/key_field/key_field_sample_generated.h (C/C++)

Code before:
  static const flatbuffers::TypeTable *MiniReflectTypeTable() {
    return BazTypeTable();
  }
  Baz()
      : a_(),
        b_(0) {
  }
  Baz(uint8_t _b)
      : a_(),
        b_(flatbuffers::EndianScalar(_b)) {
  }
  Baz(flatbuffers::span<const uint8_t, 4> _a, uint8_t _b)
      : b_(flatbuffers::EndianScalar(_b)) {
    flatbuffers::CastToArray(a_).CopyFromSpan(_a);
  }
  const flatbuffers::Array<uint8_t, 4> *a() const {
    return &flatbuffers::CastToArray(a_);
  }
  flatbuffers::Array<uint8_t, 4> *mutable_a() {
    return &flatbuffers::CastToArray(a_);
  }
  bool KeyCompareLessThan(const Baz * const o) const {
    return KeyCompareWithValue(o->a()) < 0;
  }
  int KeyCompareWithValue(const flatbuffers::Array<uint8_t, 4> *_a) const {
    const auto* curr_a = a();
    for (auto i = 0; i < curr_a->size(); i++) {
      const auto a_l = curr_a->Get(i);
      const auto a_r = _a->Get(i);
      if(a_l != a_r)
        return static_cast<int>(a_l > a_r) - static_cast<int>(a_l < a_r);
    }
    return 0;
  }
  uint8_t b() const {
    return flatbuffers::EndianScalar(b_);
  }
  void mutate_b(uint8_t _b) {
    flatbuffers::WriteScalar(&b_, _b);
  }
};
FLATBUFFERS_STRUCT_END(Baz, 5);

inline bool operator==(const Baz &lhs, const Baz &rhs) {
  return
      (lhs.a() == rhs.a()) &&
      (lhs.b() == rhs.b());
}

inline bool operator!=(const Baz &lhs, const Baz &rhs) {
    return !(lhs == rhs);

Review comment (suggestion):
instead of `auto` can we put in the type name? I think we already have it due to the parameter requiring it, so it shouldn't be hard to add.

Code after:
  static const flatbuffers::TypeTable *MiniReflectTypeTable() {
    return BazTypeTable();
  }
  Baz()
      : a_(),
        b_(0) {
  }
  Baz(uint8_t _b)
      : a_(),
        b_(flatbuffers::EndianScalar(_b)) {
  }
  Baz(flatbuffers::span<const uint8_t, 4> _a, uint8_t _b)
      : b_(flatbuffers::EndianScalar(_b)) {
    flatbuffers::CastToArray(a_).CopyFromSpan(_a);
  }
  const flatbuffers::Array<uint8_t, 4> *a() const {
    return &flatbuffers::CastToArray(a_);
  }
  flatbuffers::Array<uint8_t, 4> *mutable_a() {
    return &flatbuffers::CastToArray(a_);
  }
  bool KeyCompareLessThan(const Baz * const o) const {
    return KeyCompareWithValue(o->a()) < 0;
  }
  int KeyCompareWithValue(const flatbuffers::Array<uint8_t, 4> *_a) const {
    const flatbuffers::Array<uint8_t, 4> *curr_a = a();
    for (flatbuffers::uoffset_t i = 0; i < curr_a->size(); i++) {
      const auto lhs = curr_a->Get(i);
      const auto rhs = _a->Get(i);
      if(lhs != rhs)
        return static_cast<int>(lhs > rhs) - static_cast<int>(lhs < rhs);
    }
    return 0;
  }
  uint8_t b() const {
    return flatbuffers::EndianScalar(b_);
  }
  void mutate_b(uint8_t _b) {
    flatbuffers::WriteScalar(&b_, _b);
  }
};
FLATBUFFERS_STRUCT_END(Baz, 5);

inline bool operator==(const Baz &lhs, const Baz &rhs) {
  return
      (lhs.a() == rhs.a()) &&
      (lhs.b() == rhs.b());
}

inline bool operator!=(const Baz &lhs, const Baz &rhs) {
    return !(lhs == rhs);
Repository: google/flatbuffers
File: tests/flatc/flatc_cpp_tests.py (Python)

Code before:
    def KeepPrefixIncludePrefixoutPath_SuperDirectoryReference(self):
        # Generate bar_with_foo that references a type in a super directory.
        flatc(
            [
                "--cpp",
                "--keep-prefix",
                "--include-prefix",
                "generated",
                "-I",
                "./",
                "-o",
                ".tmp",
                "bar/bar_with_foo.fbs",
            ]
        )

        # The include prefix should come first, with the kept prefix next.
        assert_file_and_contents(
            ".tmp/bar_with_foo_generated.h",
            [
                '#include "generated/baz/baz_generated.h"',
                '#include "generated/foo_generated.h"',
            ]
        )

    def ScopedEnumsDefault(self):
        # Scoped enums have generated MIN and MAX enumerated values by
        # default.
        flatc(["--cpp", "--scoped-enums", "foo.fbs"])

        assert_file_and_contents(
            "foo_generated.h",
            ['enum class TEST_ENUM', 
             ' SPAM = 0',
             ' EGGS = 1',
             ' MIN = SPAM',
             ' MAX = EGGS']
        )

    def ScopedEnumsNoMinMaxValues(self):
        # The --no-minmax-values flag prevents scoped enums from
        # having generated MIN and MAX enumerated values.
        flatc(["--cpp", "--scoped-enums", "--no-minmax-values", "foo.fbs"])

        assert_file_and_contents(
            "foo_generated.h",
            ['enum class TEST_ENUM', ' SPAM = 0', ' EGGS = 1'],
            doesnt_contain=[' MIN =', ' MAX =']
        )

    def PrefixedEnumsDefault(self):

Review comment (suggestion):
These are fine, but probably would be better as a c++ unit test.

The tests in this file are more testing flags that interact with multiple files/includes statements that are harder to test in a small unit test.

Code after:
    def KeepPrefixIncludePrefixoutPath_SuperDirectoryReference(self):
        # Generate bar_with_foo that references a type in a super directory.
        flatc(
            [
                "--cpp",
                "--keep-prefix",
                "--include-prefix",
                "generated",
                "-I",
                "./",
                "-o",
                ".tmp",
                "bar/bar_with_foo.fbs",
            ]
        )

        # The include prefix should come first, with the kept prefix next.
        assert_file_and_contents(
            ".tmp/bar_with_foo_generated.h",
            [
                '#include "generated/baz/baz_generated.h"',
                '#include "generated/foo_generated.h"',
            ],
            unlink=False,
        )