Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If available_gas is missing on tests it now has a default large value. #4172

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Oct 1, 2023

Also enabled static value to the param for cases we wish to specifically allow no dynamic gas intake.


This change is Reviewable

@orizi orizi requested a review from yuvalsw October 1, 2023 09:59
@orizi orizi force-pushed the orizi/available-gas-default branch 2 times, most recently from 9b8761e to 2499161 Compare October 1, 2023 10:27
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 31 of 32 files at r1, all commit messages.
Reviewable status: 31 of 32 files reviewed, 5 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-plugin/src/test_config.rs line 11 at r1 (raw file):

use super::{AVAILABLE_GAS_ATTR, IGNORE_ATTR, SHOULD_PANIC_ATTR, TEST_ATTR};
use crate::STATIC_GAS_ARG;

merge the 2 lines.

Code quote:

use super::{AVAILABLE_GAS_ATTR, IGNORE_ATTR, SHOULD_PANIC_ATTR, TEST_ATTR};
use crate::STATIC_GAS_ARG;

crates/cairo-lang-test-plugin/src/test_config.rs line 121 at r1 (raw file):

}

/// Extract the available gas from the attribute.

Suggestion:

 Extracts the available 

crates/cairo-lang-test-plugin/src/test_config.rs line 138 at r1 (raw file):

                ..
            },
        ] => literal.numeric_value(db).unwrap_or_default().to_usize(),

why unwrap_or_default and not '?'?

Code quote:

unwrap_or_default()

crates/cairo-lang-test-plugin/src/test_config.rs line 149 at r1 (raw file):

                stable_ptr: attr.id_stable_ptr.untyped(),
                message: format!(
                    "Attribute should have a single value argument or `{STATIC_GAS_ARG}`."

We don't support short strings here.

Suggestion:

single numeric literal argumen

crates/cairo-lang-test-runner/src/test_config.rs line 0 at r1 (raw file):
why is this file deleted?

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 31 of 32 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)


crates/cairo-lang-test-plugin/src/test_config.rs line 138 at r1 (raw file):

Previously, yuvalsw wrote…

why unwrap_or_default and not '?'?

adding all this to the same diagnostic instead.


crates/cairo-lang-test-runner/src/test_config.rs line at r1 (raw file):

Previously, yuvalsw wrote…

why is this file deleted?

because it was duplicated - it is an old duplication of the other test_config.rs

@orizi orizi force-pushed the orizi/available-gas-default branch from 2499161 to 0d310e3 Compare October 1, 2023 11:56
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 32 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-plugin/src/test_config.rs line 122 at r2 (raw file):

/// Extract the available gas from the attribute.
/// Adds a diagnostic if the attribute is malformed.
/// Returns `None` if the attribute is `static`, or the attribute is malformed.

Suggestion:

e is "static", or

crates/cairo-lang-test-plugin/src/test_config.rs line 137 at r2 (raw file):

            stable_ptr: attr.args_stable_ptr.untyped(),
            message: format!(
                "Attribute should have a single value argument or `{STATIC_GAS_ARG}`."

numeric literal

Code quote:

 a single value argument 

crates/cairo-lang-test-plugin/src/test_config.rs line 141 at r2 (raw file):

        })
    };
    match &attr.args[..] {

consider returning a bool from this match and then if true, add the diagnostic. Otherwise, in most cases the lambda is created but not used.

Also enabled `static` value to the param for cases we wish to
specifically allow no dynamic gas intake.
@orizi orizi force-pushed the orizi/available-gas-default branch from 0d310e3 to ed246e0 Compare October 1, 2023 13:25
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 31 of 32 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)


crates/cairo-lang-test-plugin/src/test_config.rs line 137 at r2 (raw file):

Previously, yuvalsw wrote…

numeric literal

Done.


crates/cairo-lang-test-plugin/src/test_config.rs line 141 at r2 (raw file):

Previously, yuvalsw wrote…

consider returning a bool from this match and then if true, add the diagnostic. Otherwise, in most cases the lambda is created but not used.

doesn't look nicer unfortunately - rather have this.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-test-plugin/src/test_config.rs line 141 at r2 (raw file):

Previously, orizi wrote…

doesn't look nicer unfortunately - rather have this.

doesn't look nicer - maybe, but more efficient in the good (common) case.

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


crates/cairo-lang-test-plugin/src/test_config.rs line 141 at r2 (raw file):

Previously, yuvalsw wrote…

doesn't look nicer - maybe, but more efficient in the good (common) case.

the closure created and not being used is very unlikely to have an actual cost.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-test-plugin/src/test_config.rs line 141 at r2 (raw file):

Previously, orizi wrote…

the closure created and not being used is very unlikely to have an actual cost.

I still think it's better and don't see the big advantage of this approach, but not insisting - unblocking.

@orizi orizi added this pull request to the merge queue Oct 2, 2023
Merged via the queue into main with commit efb4575 Oct 2, 2023
39 checks passed
@milancermak milancermak mentioned this pull request Oct 2, 2023
1 task
@orizi orizi deleted the orizi/available-gas-default branch October 10, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants