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

Arithmetic expressions #1083

Merged
merged 44 commits into from
Mar 25, 2022
Merged

Arithmetic expressions #1083

merged 44 commits into from
Mar 25, 2022

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Feb 11, 2022

Add support for arithmetic operators:

  • Unary +.
  • Binary +, -, *, /, %.

Specify their behavior for integer and floating-point types. Signed integer overflow is a programming error, handled in various ways. Unsigned integer overflow is specified as wrapping around, intended for hashing / crypto / PRNG use cases.

@zygoloid zygoloid marked this pull request as ready for review February 17, 2022 01:47
@zygoloid zygoloid requested a review from a team as a code owner February 17, 2022 01:47
docs/design/expressions/README.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Show resolved Hide resolved
docs/design/expressions/arithmetic.md Show resolved Hide resolved

Binary `+` performs string concatenation.

## Extensibility
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be checked in as a Carbon source file, see https://discord.com/channels/655572317891461132/707150492370862090/943638327817547816

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but I also think this can be a follow-up.

docs/design/expressions/arithmetic.md Show resolved Hide resolved
proposals/p1083.md Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
docs/design/expressions/README.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
division by even numbers. We could be more mathematically pure by refusing to
implement `<` and friends for `uN`, and similarly refusing to support `/`.
There is no single canonical total order for that ring, and because
multiplication by even numbers loses information, only odd numbers have
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking: "multiplication by even numbers loses information" doesn't help me much, because I don't find it any more self-evident than "only odd numbers have multiplicative inverses". Others may have different experiences, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "by discarding the high-order bit" to try to make it clearer why it's losing information in a way that hopefully resonates with a programmer-centric view of integers modulo 2N. I mean, I'm not sure how much I should be getting into the details here rather than just pointing to Wikipedia; maybe removing some of this text and just saying division isn't well-defined is a better plan?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added "by discarding the high-order bit" to try to make it clearer why it's losing information in a way that hopefully resonates with a programmer-centric view of integers modulo 2N.

I'm afraid that still doesn't help me much, because my naive intuition is that modular multiplication discards the high-order bits on overflow, regardless of whether the operands were even. I can sort of see what you're getting at, in that you can think of multiplication by an even number as multiplication by an odd number followed by a left-shift, but only after thinking for a bit (and being primed by the research that led me to suggest the Wikipedia link).

From this point of view, I think what's counterintuitive is that multiplication by an odd number doesn't lose information.

I mean, I'm not sure how much I should be getting into the details here rather than just pointing to Wikipedia; maybe removing some of this text and just saying division isn't well-defined is a better plan?

Yeah, I think all we really need to say here is that division isn't well-defined (I'm not sure we even need to single out even numbers), and give a citation for people who find that surprising. That's complicated by the fact that the citation talks about multiplicative inverses rather than division per se, so I think it's worth keeping enough text to connect the dots from "multiplicative inverse" to "division", but I think we can safely drop the "loses information" part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed mention of even numbers here because it distracts from the point.

proposals/p1083.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Just lightly skimming since I was seeing edits, a couple questions popped up in my head.

docs/design/expressions/README.md Show resolved Hide resolved
docs/design/expressions/README.md Outdated Show resolved Hide resolved
docs/design/expressions/README.md Show resolved Hide resolved
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

(standing LGTM)

docs/design/expressions/arithmetic.md Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Largely based on the amazing reviews by everyone else -- thansk for that!

I've not heard or seen any concerns with this direction, including from the leads, so approving.

Some wording clarifications below, but none change the meaning of the proposal, so feel free to submit whenever -- I'm happy with followups as needed here.


Binary `+` performs string concatenation.

## Extensibility
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but I also think this can be a follow-up.

program behavior correct.
- Division by zero is still not handled.

### Guarantee that all overflow errors are trapped
Copy link
Contributor

Choose a reason for hiding this comment

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

This summary doesn't for me directly reflect the first paragraph...

I think the term "trapped" here isn't a good term, both due to imprecision and unwelcoming/hostile associations. Is there a more precise, and mechanical term or phrase we could use without referring to a particular hardware construct that is named this way? Maybe "are diagnosed or produce mathematically correct results", is that too wordy? Wolud "are diagnosed" be a reasonable way to describe the various ways that program execution doesn't proceed and some dynamic error state is produced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to "Guarantee that the program never proceeds with an incorrect value after overflow", which is a bit verbose but I think is more accurate.

proposals/p1083.md Outdated Show resolved Hide resolved
proposals/p1083.md Outdated Show resolved Hide resolved
Floating-point types use IEEE 754 semantics, with round-to-nearest rounding
mode, no signaling NaNs, and no floating-point exceptions.

The `+` operator is also supported on strings, and performs concatenation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the record, I hope we can remove this in the future. I really dislike this form of concatenation. But a) I don't think its worth changes here, and b) we don't really have a robust string manipulation APi yet so there isn't really an alternative. It seems very reasonable to leave this in until we've made it convenient to avoid. =]

Anyways, nothing to do here in the proposal. =]

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 intended to remove this based on earlier push-back -- this rule is gone from the design update but I forgot to remove it here. Updated proposal to say it takes no stance on this question; #457 covers this.

zygoloid and others added 2 commits March 16, 2022 14:47
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
docs/design/expressions/arithmetic.md Outdated Show resolved Hide resolved
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
@zygoloid zygoloid merged commit 5d9ff87 into carbon-language:trunk Mar 25, 2022
Proposals automation moved this from RFC to Accepted Mar 25, 2022
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Mar 25, 2022
chandlerc added a commit that referenced this pull request Jun 28, 2022
Add support for arithmetic operators:

- Unary `+`.
- Binary `+`, `-`, `*`, `/`, `%`.

Specify their behavior for integer and floating-point types. Signed integer overflow is a programming error, handled in various ways. Unsigned integer overflow is specified as wrapping around, intended for hashing / crypto / PRNG use cases.

Co-authored-by: jonmeow <46229924+jonmeow@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal accepted Decision made, proposal accepted proposal A proposal
Projects
No open projects
Proposals
Accepted
Development

Successfully merging this pull request may close these issues.

None yet

6 participants