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

[RFC 58] Add partial arithmetic for integer types #2865

Merged
merged 4 commits into from
Aug 29, 2018
Merged

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Aug 22, 2018

This PR adds partial arithmetic functions and operators to all integer types.

Normal classes can also provide partial arithmetic operators, like +?, -?, *?, etc. by implementing add_partial, sub_partial, mul_partial etc. the same way the normal operators work on classes.

I limited this PR to +?, -?, *?, /? and %?.

Two question arised during implementation:

  • Should we extend these methods to all numeric types, especially to floating point operations, erroring whenever NaN or Inf or -Inf would be returned?
  • Should we add shl_partial, shr_partial and neg_partial too? As part of this PR?

Closes #2814

@mfelsche mfelsche added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Aug 22, 2018
as it mapped +? to partial add, not to add_partial
so in this respect, a class could not have both methods, but the integer typed need to have.
@mfelsche
Copy link
Contributor Author

I added a Division test to make sure division by zero is zero and that Int.min_value()/-1 is also zero. The latter does not seem to be the case on arm and armhf:

detailed logs of failing job

/home/pony/project/packages/builtin_test/_test.pony:1801: Assert eq failed.  Expected (0) == (-170141183460469231731687303715884105728)

I think this should be handled as a separate issue.

@jemc
Copy link
Member

jemc commented Aug 22, 2018

Two question arised during implementation

I'd suggest to stick to the scope of the original RFC for the purposes of this PR, and leave out both of these extensions. These be handled in a separate RFC.

I think this should be handled as a separate issue.

Agreed. In a separate issue, we can make sure this result is deterministic across platforms, and if someone wants the faster, nondeterministic behaviour, they can use /~ (div_unsafe).

@mfelsche
Copy link
Contributor Author

Thanks @jemc yeah, i will stick the failing division test into the new issue and remove it here. This RFCs feature is working fine it seems.

this one should be included in another PR fixing the underlying issue with
detecting overflow on division for I128 on platforms without native 128 bit int support
@mfelsche
Copy link
Contributor Author

I removed the division test and will stick this PR to the RFC scope and at some point create a new RFC to extend the partial arithmetic both in terms of functions and in terms of scope (to include floating point).

(x/~y, x %~ y)
end


Copy link
Member

Choose a reason for hiding this comment

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

There's a few style issues in this file - everywhere where you have two blank lines, it should be one blank line. See https://github.com/ponylang/ponyc/blob/master/STYLE_GUIDE.md#blank-lines

@mfelsche
Copy link
Contributor Author

One minor drawback that this implementation contains is that previously, when implementing custom operator functions, say add, one could chose between a partial add(other: T)? or a total add(other: T), now, for the operators touched here, there is no partial add, just add_partial. This is not super-intuitive and needs explaining. But i cannot come up with a better way to offer both total and partial add for integers. But imho this is the only case where having both versions makes sense.

@jemc jemc merged commit 85efc78 into master Aug 29, 2018
ponylang-main added a commit that referenced this pull request Aug 29, 2018
@mfelsche mfelsche deleted the partial-arithmetic branch August 29, 2018 19:39
@SeanTAllen
Copy link
Member

Was the "how we teach this" from the RFC merged? I don't see anything re:

"The tutorial should introduce the different ways of doing arithmetic in greater detail, checked (addc etc.), unchecked (add etc.). unsafe ( add_unsafe etc.) and partial (add_partial). So that users know what options they have and what the drawbacks might be (e.g. performance against correctness)."

We need to get that in for the RFC to be considered closed. As it is, the RFC isn't finished.

@SeanTAllen
Copy link
Member

Also, this wasn't noted in Last Week in Pony.

@SeanTAllen
Copy link
Member

@mfelsche can you add release notes for this?

@SeanTAllen SeanTAllen mentioned this pull request Sep 4, 2018
@mfelsche
Copy link
Contributor Author

mfelsche commented Sep 5, 2018

The tutorial still needs to be updated. I am working on this in the little sparetime i have. ETA is t + 3-5 days.

This wasnt noted on lwip as it is not done yet, as you correctly stated.

@SeanTAllen
Copy link
Member

@mfelsche can you add release notes for this?

@mfelsche
Copy link
Contributor Author

Release Notes Draft

With this RFC Pony finally adds the means to do partial arithmetic on integers. It introduces a new set of operators: +?, -?, *?, /? and %? that all raise an error when they detect overflow/underflow or division by zero. The non-partial versions all wrapped around on overflow/underflow or defined division by zero to be 0, if this is not acceptable for you, you now can make use these operators. And if erroring is not your way, you can go with addc, subc, mulc, divc or remc which return the result together with a flag denoting overflow/underflow or division by zero. Check out the full story on the tutorial: https://tutorial.ponylang.io/expressions/arithmetic.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants