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

test(flatten_cfg): unit test and end to end test for #1792 #1806

Closed
wants to merge 9 commits into from

Conversation

ludamad
Copy link
Collaborator

@ludamad ludamad commented Jun 22, 2023

Description

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@ludamad ludamad requested a review from kevaundray June 22, 2023 22:26
// constrain v48
// return
// }
let test_set_id = Id::test_new(7);
Copy link
Collaborator Author

@ludamad ludamad Jun 22, 2023

Choose a reason for hiding this comment

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

GPT4 did the conversion after given one example, plus a bit of hand-massaging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simplified (by GPT4 of course I'm not a sucker)

@ludamad
Copy link
Collaborator Author

ludamad commented Jun 23, 2023

@kevaundray @jfecher figured out that the simplify code thinks

        // fn main f1 {
        //     ... simplified ...
        //     v_false = u1 0
        //     jmpif v_false then: b_useless_branch, else: b_fallthrough
        //   b_useless_branch():
        //     jmp b_fallthrough()
        //   b_fallthrough():
        //     constrain v_false // was incorrectly removed
        //     return
        // }

the constrain here can be removed on flatten_cfg, but I didn't debug further.

@jfecher
Copy link
Contributor

jfecher commented Jun 23, 2023

Can we open this as an issue in the future? Unless the PR already has the fix for the bug included I think it is better to file the bug first.

@ludamad
Copy link
Collaborator Author

ludamad commented Jun 23, 2023

Sure, although surely this needs the fix any way and then it has an issue? This can't be merged without it. I thought of this more as a hand off PR

@jfecher
Copy link
Contributor

jfecher commented Jun 23, 2023

Sure, although surely this needs the fix any way and then it has an issue? This can't be merged without it. I thought of this more as a hand off PR

Sorry, I misinterpreted this PR at first. I didn't see the existing linked issue #1792 somehow. I think for cases like this we can include any testing code in the issue and just make sure to add it in the same PR the issue is fixed in. That way we have assurance the PR fixes the issue in question compared to adding the test afterward.

@ludamad
Copy link
Collaborator Author

ludamad commented Jun 23, 2023

Really I should have probably slapped DO NOT MERGE on this, it was meant to be used as you did, cherry-picked into the fix PR. And it is in the issue by virtue of mentioning the issue, but I'll put it in the body in the future

@ludamad ludamad closed this Jun 23, 2023
@ludamad ludamad deleted the adam/test/1792 branch November 27, 2023 21:27
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