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

Removed faulty diagnostic on inlining over generated functions. #6257

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Aug 22, 2024

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/inline/mod.rs line 40 at r1 (raw file):

    let FunctionWithBodyLongId::Semantic(semantic_function_id) = function_id.lookup_intern(db)
    else {
        // Diagnostics are not relevant for generated functions.

why not?

Code quote:

    // Diagnostics are not relevant for generated functions.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a 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 {},

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: 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.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/inline/mod.rs line 40 at r1 (raw file):

Previously, orizi wrote…

generated functions are not marked with inline(always) ever - as this is an attribute on an item.

What is the "faulty" diagnostic?

Also, what happens if we add more inline diagnostic in the future and they are relevant for generated code?

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: 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.

@orizi orizi changed the base branch from spr/main/5b2c0eeb to main August 22, 2024 07:18
@orizi orizi enabled auto-merge August 22, 2024 07:23
@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-lowering/src/inline/mod.rs line 40 at r1 (raw file):

Previously, orizi wrote…

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.

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 
}

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: 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.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

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)

@orizi orizi added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 4cf5fa3 Aug 22, 2024
86 of 87 checks passed
@orizi orizi deleted the spr/main/f76fc778 branch August 28, 2024 05:59
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