-
Notifications
You must be signed in to change notification settings - Fork 548
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
Removed faulty diagnostic on inlining over generated functions. #6257
Conversation
why not? Code quote: // Diagnostics are not relevant for generated functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @orizi)
crates/cairo-lang-sierra-generator/src/ap_change_test_data/tests
line 85 at r1 (raw file):
fn cycle_if_some(x: Option<felt252>) { match x { Option::Some(x) => while x != 0 {},
why are you changing the test?
Code quote:
Option::Some(x) => while x != 0 {},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/inline/mod.rs
line 40 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
why not?
generated functions are not marked with inline(always)
ever - as this is an attribute on an item.
crates/cairo-lang-sierra-generator/src/ap_change_test_data/tests
line 85 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
why are you changing the test?
that was the actual way i wanted to write the test.
Previously, orizi wrote…
What is the "faulty" diagnostic? Also, what happens if we add more inline diagnostic in the future and they are relevant for generated code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/inline/mod.rs
line 40 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
What is the "faulty" diagnostic?
Also, what happens if we add more inline diagnostic in the future and they are relevant for generated code?
You cannot write loops within a always inline func.
If we ever mark closures or loops with inline always we would require a fix.
But I don't really think we'd ever be doing that.
826f3d6
to
eb8e25f
Compare
Previously, orizi wrote…
can you change it to Code snippet: let inline_config = match function_id.lookup_intern(db) {
semantic => db.function_declaration_inline_config(semantic_function_id)?
generated => InlineConfiguration::None
} |
commit-id:f76fc778
eb8e25f
to
49156e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)
crates/cairo-lang-lowering/src/inline/mod.rs
line 40 at r1 (raw file):
Previously, ilyalesokhin-starkware wrote…
can you change it to
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @orizi)
Stack:
This change is