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.

Sign up for GitHub

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

Jump to bottom

[move compiler] add named blocks #14577

Merged
merged 6 commits into from
Nov 29, 2023
Merged

[move compiler] add named blocks #14577

merged 6 commits into from
Nov 29, 2023

Conversation

cgswords
Copy link
Contributor

Description

This adds named blocks for loop, while, and normal block positions.

Test Plan

A number of new tests.

If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@vercel Vercel
Copy link

vercel bot commented Oct 31, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 0:13am
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 0:13am
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 0:13am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Nov 29, 2023 0:13am
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 29, 2023 0:13am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 29, 2023 0:13am

@cgswords cgswords force-pushed the cgswords/named_blocks branch from d390134 to 8e96ef4 Compare October 31, 2023 22:31
@vercel vercel bot temporarily deployed to Preview – mysten-ui October 31, 2023 22:31 Inactive
cgswords
cgswords commented Oct 31, 2023
View reviewed changes
external-crates/move/move-compiler/tests/move_check/typing/large_binop.move Outdated
(255 - 255 == 0) ||
(255 & 255 == 255) &&
(255 | 255 == 255) ||
(255 ^ 255 == 0) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to drop just a few of these to get the compiler through name resolution due to the context size increase. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow track getting back to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a FIXME. If you'd like me to add an issue as well, I will. Since it's a fairly-invasive rewrite, I'd rather postpone it for another diff.

@cgswords cgswords force-pushed the cgswords/named_blocks branch from 8e96ef4 to 7204e48 Compare November 2, 2023 17:58
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 2, 2023 17:58 Inactive
@cgswords cgswords marked this pull request as ready for review November 2, 2023 21:14
@cgswords cgswords requested review from tnowacki, tzakian and amnn November 2, 2023 21:14
@cgswords cgswords force-pushed the cgswords/named_blocks branch from 80ea966 to cd0c1fd Compare November 3, 2023 22:34
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 3, 2023 22:36 Inactive
tnowacki
tnowacki requested changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

I haven't looked through tests extensively yet (though I think we will need some more of them :)) I just wanted to get you some feedback ASAP, sorry for the delay.
Overall,l ooks great! Thanks for turning this out so quickly. Mostly some nits/small concerns. The blocking ones being:

  • I think used_labels is unused?
  • We need a better error message for incorrect label usage

Not blocking but, I think we should gate this behind 2024? ... Maybe? ... maybe not I don't know anymore lol

@cgswords cgswords force-pushed the cgswords/named_blocks branch from 7919415 to 6a7ea7c Compare November 14, 2023 22:36
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 14, 2023 22:37 Inactive
Base automatically changed from compiler-dev to main November 22, 2023 00:12
@cgswords cgswords force-pushed the cgswords/named_blocks branch from be679c3 to 30b6e71 Compare November 22, 2023 20:08
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 22, 2023 20:09 Inactive
@cgswords cgswords force-pushed the cgswords/named_blocks branch from 9b8170b to 29e383a Compare November 27, 2023 23:58
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 27, 2023 23:59 Inactive
@cgswords cgswords force-pushed the cgswords/named_blocks branch from 29e383a to 315c0e8 Compare November 28, 2023 19:43
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 28, 2023 19:44 Inactive
@cgswords cgswords force-pushed the cgswords/named_blocks branch from 315c0e8 to e22f515 Compare November 29, 2023 00:11
@vercel vercel bot temporarily deployed to Preview – mysten-ui November 29, 2023 00:13 Inactive
@cgswords cgswords merged commit 892ab7c into main Nov 29, 2023
@cgswords cgswords deleted the cgswords/named_blocks branch November 29, 2023 20:27
@cgswords cgswords mentioned this pull request Jan 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnowacki tnowacki tnowacki approved these changes

@tzakian tzakian Awaiting requested review from tzakian

@amnn amnn Awaiting requested review from amnn

@oxade oxade Awaiting requested review from oxade

@wlmyng wlmyng Awaiting requested review from wlmyng

@stefan-mysten stefan-mysten Awaiting requested review from stefan-mysten

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgswords @tnowacki