ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: utils/wasm-builder/src/lib.rs (Rust)

Code before:
			if cargo_version_other.is_nightly && cargo_version.is_stable() || is_older {
				*rustup_version_other = rustup_version.into();
				*cargo_version_other = cargo_version;
			}
		} else {
			found_version = Some((rustup_version.into(), cargo_version));
		}
	}

	let version = found_version?.0;
	Some(CargoCommand::new_with_args("rustup", &["run", &version, "cargo"]))
}

/// The version of rustc/cargo.
#[derive(Clone, Copy, Debug)]
struct Version {
	pub major: u32,
	pub minor: u32,
	pub patch: u32,
	pub is_nightly: bool,
}

impl Version {
	/// Returns if `self` is older than `other`.
	fn is_older(&self, other: &Self) -> bool {
		self.major < other.major || self.minor < other.minor || self.patch < other.patch
	}

	/// Returns if `self` is a stable version.
	fn is_stable(&self) -> bool {
		!self.is_nightly
	}
}

/// Wraps a specific command which represents a cargo invocation.
#[derive(Debug)]
struct CargoCommand {
	program: String,
	args: Vec<String>,
	version: Option<Version>,
}

impl CargoCommand {
	fn new(program: &str) -> Self {
		let version = Self::extract_version(program, &[]);

		CargoCommand { program: program.into(), args: Vec::new(), version }
	}

	fn new_with_args(program: &str, args: &[&str]) -> Self {
		let version = Self::extract_version(program, args);

Review comment (suggestion):
3.2.1 - 2.4.1 will report `true` even though 2.4.1 is older. I think this should be smth like

```
self.major < other.major || (self.major == other.major && self.minor < other.minor) || (self.major == other.major && self.minor == other.minor && self.patch < other.patch)
```

Code after:
	Some(CargoCommand::new_with_args("rustup", &["run", &version, "cargo"]))
}

/// Wraps a specific command which represents a cargo invocation.
#[derive(Debug)]
struct CargoCommand {
	program: String,
	args: Vec<String>,
	version: Option<Version>,
}

impl CargoCommand {
	fn new(program: &str) -> Self {
		let version = Self::extract_version(program, &[]);

		CargoCommand { program: program.into(), args: Vec::new(), version }
	}

	fn new_with_args(program: &str, args: &[&str]) -> Self {
		let version = Self::extract_version(program, args);

		CargoCommand {
			program: program.into(),
			args: args.iter().map(ToString::to_string).collect(),
			version,
		}
	}

	fn command(&self) -> Command {
		let mut cmd = Command::new(&self.program);
		cmd.args(&self.args);
		cmd
	}

	fn extract_version(program: &str, args: &[&str]) -> Option<Version> {
		let version = Command::new(program)
			.args(args)
			.arg("--version")
			.output()
			.ok()
			.and_then(|o| String::from_utf8(o.stdout).ok())?;

		Version::extract(&version)
	}

	/// Returns the version of this cargo command or `None` if it failed to extract the version.
	fn version(&self) -> Option<Version> {
		self.version
	}

	/// Check if the supplied cargo command supports our Substrate wasm environment.
Repository: paritytech/substrate
File: utils/wasm-builder/src/version.rs (Rust)

Code before:
pub struct Version {
	pub major: u32,
	pub minor: u32,
	pub patch: u32,
	pub is_nightly: bool,
	pub year: u32,
	pub month: u32,
	pub day: u32,
}

impl Version {
	/// Returns if `self` is a stable version.
	pub fn is_stable(&self) -> bool {
		!self.is_nightly
	}

	/// Return if `self` is a nightly version.
	pub fn is_nightly(&self) -> bool {
		self.is_nightly
	}

	/// Extract from the given `version` string.
	pub fn extract(version: &str) -> Option<Self> {
		let mut is_nightly = false;
		let version_parts = version
			.split(" ")
			.nth(1)?
			.split(".")
			.filter_map(|v| {
				if let Some(rest) = v.strip_suffix("-nightly") {
					is_nightly = true;
					rest.parse().ok()
				} else {
					v.parse().ok()
				}
			})
			.collect::<Vec<u32>>();

		if version_parts.len() != 3 {
			return None
		}

		let date = version.split(" ").nth(3)?;

		let date_parts = date
			.split("-")
			.filter_map(|v| v.trim().strip_suffix(")").unwrap_or(v).parse().ok())
			.collect::<Vec<u32>>();

		if date_parts.len() != 3 {
			return None

Review comment (suggestion):
```suggestion
		let version_parts = version
			.trim()
			.split(" ")
```

Not sure if it matters, but when I'm doing any string processing (especially when spawning child processes) I always like to trim any potential trailing/leading whitespace just in case.

Code after:
pub struct Version {
	pub major: u32,
	pub minor: u32,
	pub patch: u32,
	pub is_nightly: bool,
	pub year: u32,
	pub month: u32,
	pub day: u32,
}

impl Version {
	/// Returns if `self` is a stable version.
	pub fn is_stable(&self) -> bool {
		!self.is_nightly
	}

	/// Return if `self` is a nightly version.
	pub fn is_nightly(&self) -> bool {
		self.is_nightly
	}

	/// Extract from the given `version` string.
	pub fn extract(version: &str) -> Option<Self> {
		let mut is_nightly = false;
		let version_parts = version
			.trim()
			.split(" ")
			.nth(1)?
			.split(".")
			.filter_map(|v| {
				if let Some(rest) = v.strip_suffix("-nightly") {
					is_nightly = true;
					rest.parse().ok()
				} else {
					v.parse().ok()
				}
			})
			.collect::<Vec<u32>>();

		if version_parts.len() != 3 {
			return None
		}

		let date = version.split(" ").nth(3)?;

		let date_parts = date
			.split("-")
			.filter_map(|v| v.trim().strip_suffix(")").unwrap_or(v).parse().ok())
			.collect::<Vec<u32>>();

		if date_parts.len() != 3 {
Repository: paritytech/substrate
File: bin/node-template/pallets/template/src/benchmarking.rs (Rust)

Code before:
//! Benchmarking setup for pallet-template

use super::*;

#[allow(unused)]
use crate::Pallet as Template;
use frame_benchmarking::v1::{benchmarks, whitelisted_caller};
use frame_system::RawOrigin;

benchmarks! {
	do_something {
		let s in 0 .. 100;
		let caller: T::AccountId = whitelisted_caller();
	}: _(RawOrigin::Signed(caller), s)
	verify {
		assert_eq!(Something::<T>::get(), Some(s));
	}

	cause_error {
		Something::<T>::put(100u32);
	}: _(RawOrigin::Signed(whitelisted_caller()))
	verify {
		assert_eq!(Something::<T>::get(), Some(101u32));
	}

	impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test);
}


Review comment (suggestion):
Yes looks good so far. Maybe you can also convert them to use the V2 syntax since they are quite short? [Example](https://github.com/paritytech/substrate/blob/56de870f943679fede8443d06d3aa96a9f7321d6/frame/examples/basic/src/benchmarking.rs#L43)

Code after:
//! Benchmarking setup for pallet-template
#![cfg(feature = "runtime-benchmarks")]
use super::*;

#[allow(unused)]
use crate::Pallet as Template;
use frame_benchmarking::v2::*;
use frame_system::RawOrigin;

#[benchmarks]
mod benchmarks {
	use super::*;

	#[benchmark]
	fn do_something() {
		let value = 100u32.into();
		let caller: T::AccountId = whitelisted_caller();
		#[extrinsic_call]
		do_something(RawOrigin::Signed(caller), value);

		assert_eq!(Something::<T>::get(), Some(value));
	}

	#[benchmark]
	fn cause_error() {
		Something::<T>::put(100u32);
		let caller: T::AccountId = whitelisted_caller();
		#[extrinsic_call]
		cause_error(RawOrigin::Signed(caller));

		assert_eq!(Something::<T>::get(), Some(101u32));
	}

	impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test);
}
Repository: paritytech/substrate
File: client/cli/src/params/runtime_params.rs (Rust)

Code before:
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use clap::Args;
use std::str::FromStr;

/// Parameters used to config runtime.
#[derive(Debug, Clone, Args)]
pub struct RuntimeParams {
	/// The size of the instances cache for each runtime. The values higher than 256 are illegal.
	#[arg(long, default_value_t = 8, value_parser = parse_max_runtime_instances)]
	pub max_runtime_instances: usize,

	/// Maximum number of different runtimes that can be cached.
	#[arg(long, default_value_t = 2)]
	pub runtime_cache_size: u8,
}

fn parse_max_runtime_instances(s: &str) -> Result<usize, String> {
	let max_runtime_instances = usize::from_str(s)
		.map_err(|_err| format!("Illegal `--max-runtime-instances` value: {s}"))?;

	if max_runtime_instances > 256 {
		Err(format!("Illegal `--max-runtime-instances` value: {max_runtime_instances}"))
	} else {
		Ok(max_runtime_instances)
	}
}


Review comment (suggestion):
```suggestion
		Err(format!("Illegal `--max-runtime-instances` value: {max_runtime_instances} is more than the allowed maximum of `256` "))
```

Code after:
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use clap::Args;
use std::str::FromStr;

/// Parameters used to config runtime.
#[derive(Debug, Clone, Args)]
pub struct RuntimeParams {
	/// The size of the instances cache for each runtime. The values higher than 256 are illegal.
	#[arg(long, default_value_t = 8, value_parser = parse_max_runtime_instances)]
	pub max_runtime_instances: usize,

	/// Maximum number of different runtimes that can be cached.
	#[arg(long, default_value_t = 2)]
	pub runtime_cache_size: u8,
}

fn parse_max_runtime_instances(s: &str) -> Result<usize, String> {
	let max_runtime_instances = usize::from_str(s)
		.map_err(|_err| format!("Illegal `--max-runtime-instances` value: {s}"))?;

	if max_runtime_instances > 256 {
		Err(format!("Illegal `--max-runtime-instances` value: {max_runtime_instances} is more than the allowed maximum of `256` "))
	} else {
		Ok(max_runtime_instances)
	}
}
Repository: paritytech/substrate
File: frame/sudo/src/lib.rs (Rust)

Code before:
//! The Sudo pallet depends on the [`GenesisConfig`].
//! You need to set an initial superuser account as the sudo `key`.
//!
//! ## Related Pallets
//!
//! * [Democracy](../pallet_democracy/index.html)
//!
//! [`Origin`]: https://docs.substrate.io/main-docs/build/origins/

#![cfg_attr(not(feature = "std"), no_std)]

use sp_runtime::{traits::StaticLookup, DispatchResult};
use sp_std::prelude::*;

use frame_support::{dispatch::GetDispatchInfo, traits::UnfilteredDispatchable};

mod extension;
#[cfg(test)]
mod mock;
#[cfg(test)]
mod tests;

#[cfg(feature = "runtime-benchmarks")]
mod benchmarking;
pub mod weights;
pub use weights::*;

pub use extension::CheckOnlySudoAccount;
pub use pallet::*;

type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

#[frame_support::pallet]
pub mod pallet {
	use super::{DispatchResult, *};
	use frame_support::pallet_prelude::*;
	use frame_system::pallet_prelude::*;

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// The overarching event type.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		/// A sudo-able call.
		type RuntimeCall: Parameter
			+ UnfilteredDispatchable<RuntimeOrigin = Self::RuntimeOrigin>
			+ GetDispatchInfo;

		/// Type representing the weight of this pallet
		type WeightInfo: WeightInfo;
	}

Review comment (suggestion):
```suggestion
```

I dont think we normally do this.

Code after:
//! The Sudo pallet depends on the [`GenesisConfig`].
//! You need to set an initial superuser account as the sudo `key`.
//!
//! ## Related Pallets
//!
//! * [Democracy](../pallet_democracy/index.html)
//!
//! [`Origin`]: https://docs.substrate.io/main-docs/build/origins/

#![cfg_attr(not(feature = "std"), no_std)]

use sp_runtime::{traits::StaticLookup, DispatchResult};
use sp_std::prelude::*;

use frame_support::{dispatch::GetDispatchInfo, traits::UnfilteredDispatchable};

mod extension;
#[cfg(test)]
mod mock;
#[cfg(test)]
mod tests;

#[cfg(feature = "runtime-benchmarks")]
mod benchmarking;
pub mod weights;
pub use weights::WeightInfo;

pub use extension::CheckOnlySudoAccount;
pub use pallet::*;

type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

#[frame_support::pallet]
pub mod pallet {
	use super::{DispatchResult, *};
	use frame_support::pallet_prelude::*;
	use frame_system::pallet_prelude::*;

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// The overarching event type.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		/// A sudo-able call.
		type RuntimeCall: Parameter
			+ UnfilteredDispatchable<RuntimeOrigin = Self::RuntimeOrigin>
			+ GetDispatchInfo;

		/// Type representing the weight of this pallet
		type WeightInfo: WeightInfo;
	}
Repository: paritytech/substrate
File: frame/sudo/src/benchmarking.rs (Rust)

Code before:
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// 	http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Benchmarks for Sudo Pallet

use super::*;
use crate::Pallet;
use frame_benchmarking::v2::*;
use frame_system::RawOrigin;

const SEED: u32 = 0;

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
	frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}

#[benchmarks]
mod benchmarks {
	use super::*;

	#[benchmark]
	fn set_key() {
		let caller: T::AccountId = whitelisted_caller();
		Key::<T>::put(caller.clone());

		let new_sudoer: T::AccountId = account("sudoer", 0, SEED);
		let new_sudoer_lookup = T::Lookup::unlookup(new_sudoer.clone());

		#[extrinsic_call]

Review comment (suggestion):
```suggestion
//! Benchmarks for Sudo Pallet
```

Code after:
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// 	http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Benchmarks for Sudo Pallet

use super::*;
use crate::Pallet;
use frame_benchmarking::v2::*;
use frame_system::RawOrigin;

const SEED: u32 = 0;

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
	frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}

#[benchmarks( where <T as Config>::RuntimeCall: From<frame_system::Call<T>>)]
mod benchmarks {
	use super::*;

	#[benchmark]
	fn set_key() {
		let caller: T::AccountId = whitelisted_caller();
		Key::<T>::put(caller.clone());

		let new_sudoer: T::AccountId = account("sudoer", 0, SEED);
		let new_sudoer_lookup = T::Lookup::unlookup(new_sudoer.clone());

		#[extrinsic_call]
Repository: paritytech/substrate
File: frame/sudo/src/lib.rs (Rust)

Code before:
#[cfg(feature = "runtime-benchmarks")]
mod benchmarking;
pub mod weights;
pub use weights::*;

pub use extension::CheckOnlySudoAccount;
pub use pallet::*;

type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

#[frame_support::pallet]
pub mod pallet {
	use super::{DispatchResult, *};
	use frame_support::pallet_prelude::*;
	use frame_system::pallet_prelude::*;

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// The overarching event type.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		/// A sudo-able call.
		type RuntimeCall: Parameter
			+ UnfilteredDispatchable<RuntimeOrigin = Self::RuntimeOrigin>
			+ GetDispatchInfo
			+ From<frame_system::Call<Self>>;

		/// Type representing the weight of this pallet
		type WeightInfo: WeightInfo;
	}

	#[pallet::pallet]
	pub struct Pallet<T>(PhantomData<T>);

	#[pallet::call]
	impl<T: Config> Pallet<T> {
		/// Authenticates the sudo key and dispatches a function call with `Root` origin.
		///
		/// The dispatch origin for this call must be _Signed_.
		///
		/// ## Complexity
		/// - O(1).
		#[pallet::call_index(0)]
		#[pallet::weight({
			let dispatch_info = call.get_dispatch_info();
			(dispatch_info.weight, dispatch_info.class)
		})]
		pub fn sudo(
			origin: OriginFor<T>,
			call: Box<<T as Config>::RuntimeCall>,
		) -> DispatchResultWithPostInfo {

Review comment (bug):
I think its not wrong, since we already require the system pallet as config super trait, but also not that nice looking.  
You can put a `where` on the outer benchmark macro.

Code after:
#[cfg(feature = "runtime-benchmarks")]
mod benchmarking;
pub mod weights;
pub use weights::WeightInfo;

pub use extension::CheckOnlySudoAccount;
pub use pallet::*;

type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

#[frame_support::pallet]
pub mod pallet {
	use super::{DispatchResult, *};
	use frame_support::pallet_prelude::*;
	use frame_system::pallet_prelude::*;

	#[pallet::config]
	pub trait Config: frame_system::Config {
		/// The overarching event type.
		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

		/// A sudo-able call.
		type RuntimeCall: Parameter
			+ UnfilteredDispatchable<RuntimeOrigin = Self::RuntimeOrigin>
			+ GetDispatchInfo;

		/// Type representing the weight of this pallet
		type WeightInfo: WeightInfo;
	}

	#[pallet::pallet]
	pub struct Pallet<T>(PhantomData<T>);

	#[pallet::call]
	impl<T: Config> Pallet<T> {
		/// Authenticates the sudo key and dispatches a function call with `Root` origin.
		///
		/// The dispatch origin for this call must be _Signed_.
		///
		/// ## Complexity
		/// - O(1).
		#[pallet::call_index(0)]
		#[pallet::weight({
			let dispatch_info = call.get_dispatch_info();
			(
				T::WeightInfo::sudo().saturating_add(dispatch_info.weight),
				dispatch_info.class
			)
		})]
		pub fn sudo(
			origin: OriginFor<T>,
Repository: paritytech/substrate
File: frame/offences/src/migration.rs (Rust)

Code before:
		ValueQuery,
	>;
}

pub mod v1 {
	use super::*;

	pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
		#[cfg(feature = "try-runtime")]
		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
			let onchain = Pallet::<T>::on_chain_storage_version();

			ensure!(onchain < 1, "this migration can be deleted");

			log::info!(target: LOG_TARGET,  "Number of reports to refund and delete: {}", ReportsByKindIndex::<T>::iter().count());

			Ok(Vec::new())
		}

		fn on_runtime_upgrade() -> Weight {
			let current = Pallet::<T>::current_storage_version();
			let onchain = Pallet::<T>::on_chain_storage_version();

			if onchain > 0 {
				log::info!("MigrateToV1 should be removed");
				return T::DbWeight::get().reads(1)
			}

			let status = v0::ReportsByKindIndex::<T>::drain().collect::<Vec<_>>();
			let weight = T::DbWeight::get().reads(status.len() as u64);

			current.put::<Pallet<T>>();

			weight
		}

		#[cfg(feature = "try-runtime")]
		fn post_upgrade(_state: Vec<u8>) -> Result<(), &'static str> {
			let onchain = Pallet::<T>::on_chain_storage_version();
			ensure!(onchain < 2, "this migration needs to be removed");
			ensure!(onchain == 1, "this migration needs to be run");
			ensure!(
				ReportsByKindIndex::<T>::iter().count() == 0,
				"there are some dangling reports that need to be destroyed and refunded"
			);
			Ok(())
		}
	}
}


Review comment (suggestion):
```suggestion
				log::info!(target: LOG_TARGET, "pallet_offences::MigrateToV1 should be removed");
```

Code after:

	#[storage_alias]
	pub type ReportsByKindIndex<T: Config> = StorageMap<
		Pallet<T>,
		Twox64Concat,
		Kind,
		Vec<u8>, // (O::TimeSlot, ReportIdOf<T>)
		ValueQuery,
	>;
}

pub mod v1 {
	use frame_support::traits::StorageVersion;

	use super::*;

	pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
		#[cfg(feature = "try-runtime")]
		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
			let onchain = Pallet::<T>::on_chain_storage_version();
			ensure!(onchain < 1, "pallet_offences::MigrateToV1 migration can be deleted");

			log::info!(
				target: LOG_TARGET,
				"Number of reports to refund and delete: {}",
				v0::ReportsByKindIndex::<T>::iter_keys().count()
			);

			Ok(Vec::new())
		}

		fn on_runtime_upgrade() -> Weight {
			let onchain = Pallet::<T>::on_chain_storage_version();

			if onchain > 0 {
				log::info!(target: LOG_TARGET, "pallet_offences::MigrateToV1 should be removed");
				return T::DbWeight::get().reads(1)
			}

			let keys_removed = v0::ReportsByKindIndex::<T>::clear(u32::MAX, None).unique as u64;
			let weight = T::DbWeight::get().reads_writes(keys_removed, keys_removed);

			StorageVersion::new(1).put::<Pallet<T>>();

			weight
		}

		#[cfg(feature = "try-runtime")]
		fn post_upgrade(_state: Vec<u8>) -> Result<(), &'static str> {
			let onchain = Pallet::<T>::on_chain_storage_version();