Skip to content

Nested brackets in optimizer#11830

Closed
v-sreekesh wants to merge 3 commits intoargotorg:reformat-standard-optimizer-cmdline-testsfrom
v-sreekesh:Nested_Brackets_Optimization
Closed

Nested brackets in optimizer#11830
v-sreekesh wants to merge 3 commits intoargotorg:reformat-standard-optimizer-cmdline-testsfrom
v-sreekesh:Nested_Brackets_Optimization

Conversation

@v-sreekesh
Copy link
Contributor

@v-sreekesh v-sreekesh commented Aug 22, 2021

Fixes #11809.
Depends on #12098. Draft until that PR is merged.

@hrkrshnn hrkrshnn marked this pull request as draft August 23, 2021 08:03
@cameel cameel changed the title Nested brackets optimization Nested brackets in optimizer Aug 23, 2021
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @v-sreekesh!

Here is some feedback from me. I have a suggestion on how to approach the implementation and also some hints on how to match our style.

One other suggestion would be to put Fixes #11809 in the first comment of the PR. This way github will know that it can close the original issue once we merge the PR.

Also, please take a look at the CI results. There are compilation errors there. You'll need to fix them to make the PR mergeable. Let me know if you have any problems dealing with them.

Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Still needs some work but you're going in a good direction.

If you want to check if your changes to validation are correct, please run test/cmdlineTests.sh. It has some tests covering it already but you should also add some more and also update expectations in existing tests. For example yul_optimizer_steps_invalid_nesting should no longer expect an error after your changes.

@cameel
Copy link
Collaborator

cameel commented Sep 2, 2021

@v-sreekesh How is it going with the implementation? Do you need any help?

@v-sreekesh
Copy link
Contributor Author

@v-sreekesh How is it going with the implementation? Do you need any help?

Hi @cameel i was out of station from last two weeks will be working on this by this weekend i will let you know if i need any help thanks

@cameel cameel self-assigned this Oct 6, 2021
@cameel cameel force-pushed the Nested_Brackets_Optimization branch from 30ce4f8 to 4d582d5 Compare October 6, 2021 17:01
@cameel cameel changed the base branch from develop to reformat-standard-optimizer-cmdline-tests October 6, 2021 17:02
@cameel
Copy link
Collaborator

cameel commented Oct 6, 2021

This is now fully implemented and ready for review.

I did some test cleanup to make the changes the feature introduces easier to see. That's in #12098 and needs to be merged before this PR.

@cameel cameel force-pushed the Nested_Brackets_Optimization branch from 4d582d5 to 620ec47 Compare October 6, 2021 17:15
@chriseth chriseth deleted the branch argotorg:reformat-standard-optimizer-cmdline-tests October 7, 2021 08:22
@chriseth chriseth closed this Oct 7, 2021
@chriseth
Copy link
Contributor

chriseth commented Oct 7, 2021

Weird, why was this closed? I just merged the other PR...
I'm not able to re-open this, so created a new PR: #12102

@cameel
Copy link
Collaborator

cameel commented Oct 7, 2021

I've had this happen before. Github for some reason closes external PRs based on another branch. Maybe it's because merging the base PR deletes the branch and github can't change base to develop because the latest commits from develop do not exist in the external repo? In any case this pretty inconvenient on github's part.

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.

Nested brackets in optimizer sequences

3 participants