Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add Test for Variable Components in Benchmarking #7902

Merged
3 commits merged into from
Jan 15, 2021

Conversation

shawntabrizi
Copy link
Member

This is a test that shows an example of how you can use an expression for a lower_bound of a benchmark.

I tried to add:

	// mutation arm to look for an expression for param_from.
	(
		{ $( $instance:ident )? }
		$name:ident
		{ $( $where_clause:tt )* }
		{ $( $parsed:tt )* }
		{ $eval:block }
		{
			let $param:ident in $param_from:expr .. $param_to:expr => $param_instancer:expr ;
			$( $rest:tt )*
		}
		$postcode:block
	) => {
		$crate::benchmark_backend! {
			{ $( $instance)? }
			$name
			{ $( $where_clause )* }
			{ $( $parsed )* }
			{ $eval }
			{
				let $param in ( $param_from ) .. $param_to => $param_instancer;
				$( $rest )*
			}
			$postcode
		}
	};

But got the error:

error: `$param_from:expr` is followed by `..`, which is not allowed for `expr` fragments
   --> frame/benchmarking/src/lib.rs:480:41
    |
480 |             let $param:ident in $param_from:expr .. $param_to:expr => $param_instancer:expr ;
    |                                                  ^^ not allowed after `expr` fragments
    |
    = note: allowed there are: `=>`, `,` or `;`

So instead we take advantage of this matching arm:

	(
		{ $( $instance:ident )? }
		$name:ident
		{ $( $where_clause:tt )* }
		{ $( $parsed:tt )* }
		{ $eval:block }
		{
			let $param:ident in ( $param_from:expr ) .. $param_to:expr => $param_instancer:expr;
			$( $rest:tt )*
		}
		$postcode:block
	) => {
		$crate::benchmark_backend! {
			{ $( $instance)? }
			$name
			{ $( $where_clause )* }
			{
				$( $parsed )*
				PARAM { $param , $param_from , $param_to , $param_instancer }
			}
			{ $eval }
			{ $( $rest )* }
			$postcode
		}
	};

Where the param_from:expr is wrapped in parenthesis.

@thiolliere is there a cleaner way to do this?

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 14, 2021
@shawntabrizi shawntabrizi added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes labels Jan 14, 2021
@gui1117
Copy link
Contributor

gui1117 commented Jan 15, 2021

I don't really see other solution, you could read token by token until .. but this could break if user use .. inside the expression

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth changing this .. for 3.0.

@gui1117
Copy link
Contributor

gui1117 commented Jan 15, 2021

maybe doc could emphasize it in a note ?

@gui1117 gui1117 added the C1-low PR touches the given topic and has a low impact on builders. label Jan 15, 2021
@shawntabrizi
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Jan 15, 2021

Trying merge.

@ghost ghost merged commit 23ece2f into master Jan 15, 2021
@ghost ghost deleted the shawntabrizi-benchmark-variable-components branch January 15, 2021 14:44
@shawntabrizi
Copy link
Member Author

@kianenigma the .. syntax is pretty natural to me. What would be your alternative?

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants