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

Show deprecation on arithmetic word operations #2339

Merged
merged 57 commits into from
Feb 12, 2021
Merged

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Feb 12, 2021

next step in the plan of
#1824 (comment)

should be merged after #2326

Add a migration guide to the users guide.

nomeata and others added 30 commits February 9, 2021 12:34
this extracts, off #2309, the refactoring of `value`, without any
user-visible changes.

This should make reviewing #2309 easier, and also pave the way to
deprecating the word type.

Because this is based off #2309, it includes some changes to the prims.
In particular, the `num_conv_t1_t2` family is now for trapping
conversions, and `num_warp_t1_2` is for wrapping (currently: to Word,
and between fixed types).
Builds on top of #2324, and is extracted from #2324.

Because this should be backwards-compatible, there is still the +>>
operator. Once `WordN` is removed, we only want and need the `>>`
operator (taking the signedness from the type). Until then, `

 * `>>` is unsigned on Nat and Word, and signed on Int.
 * `+>>` is always signed
Co-authored-by: Andreas Rossberg <[email protected]>
as it is slated for removal with WordN
and some knock-on effects for `charToWord32`
Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
Commit b62a067 on origin/master has the same tree as
commit 3e5959a.
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

Comment on lines +190 to +193
| Prim (Word8|Word16|Word32|Word64), AddOp -> word_op_warn "+"
| Prim (Word8|Word16|Word32|Word64), SubOp -> word_op_warn "-"
| Prim (Word8|Word16|Word32|Word64), MulOp -> word_op_warn "*"
| Prim (Word8|Word16|Word32|Word64), PowOp -> word_op_warn "**"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do this and remove the open Type

Suggested change
| Prim (Word8|Word16|Word32|Word64), AddOp -> word_op_warn "+"
| Prim (Word8|Word16|Word32|Word64), SubOp -> word_op_warn "-"
| Prim (Word8|Word16|Word32|Word64), MulOp -> word_op_warn "*"
| Prim (Word8|Word16|Word32|Word64), PowOp -> word_op_warn "**"
| Prim T.(Word8|Word16|Word32|Word64), AddOp -> word_op_warn "+"
| Prim T.(Word8|Word16|Word32|Word64), SubOp -> word_op_warn "-"
| Prim T.(Word8|Word16|Word32|Word64), MulOp -> word_op_warn "*"
| Prim T.(Word8|Word16|Word32|Word64), PowOp -> word_op_warn "**"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, that would exclue Prim. And then you need extra parenthesis, which I like less than you do ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or put T. before those as well -- still a net win. :)

@nomeata
Copy link
Collaborator Author

nomeata commented Feb 12, 2021

Did you see the user’s guide changes or did we race there?

@rossberg
Copy link
Contributor

Ah, no, we raced indeed. LGTM as well.

Base automatically changed from joachim/new-bit-ops to master February 12, 2021 13:50
Commit 833386c on origin/master has the same tree as
commit 4b50977.
@nomeata nomeata marked this pull request as ready for review February 12, 2021 13:50
@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Feb 12, 2021
@mergify mergify bot merged commit 521e23f into master Feb 12, 2021
@mergify mergify bot deleted the joachim/warn-on-word-ops branch February 12, 2021 13:52
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 12, 2021
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.

3 participants