Skip to content

Conversation

Infiltrator
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
9717 std.math.round rounds away from zero instead of to the nearest even integer

@tsbockman
Copy link
Contributor

Looks good to me.

@ibuclaw ibuclaw added the math label Dec 5, 2015
@ibuclaw
Copy link
Member

ibuclaw commented Dec 5, 2015

👍

@ibuclaw
Copy link
Member

ibuclaw commented Dec 5, 2015

Auto-merge toggled on

ibuclaw added a commit that referenced this pull request Dec 5, 2015
fix issue 9717 - `std.math.round` rounds away from zero instead of to the nearest even integer
@ibuclaw ibuclaw merged commit 0c39d2d into dlang:master Dec 5, 2015
@denis-sh
Copy link
Contributor

denis-sh commented Dec 6, 2015

What is the point of documenting useless implementation detail?
Isn't it a correct statement that people either don't care about rounding or prefer bank rounding method? If it is correct we have just documented our round function is and always will be just as useless as C one and people who care about rounding will have to write their own function (as e.g. I did).

@denis-sh
Copy link
Contributor

denis-sh commented Dec 6, 2015

Also am I correct that people who don't care about rounding just created the pull, discussed it and merged? If you don't use some sort of mathematics stuff it doesn't mead you can remove it from language and document its bugs.

I strongly advise to review my comments and fix situation or create an appropriate explanation and post it here and in issue 9717.

@tsbockman
Copy link
Contributor

@denis-sh

What is the point of documenting useless implementation detail?

Obviously it's not a "useless" detail judging by how upset you are about it. It's important that the documentation describe what the function actually does, even if only so that people who want something different know to not to use it.

Also am I correct that people who don't care about rounding just created the pull, discussed it and merged?

No.

If you don't use some sort of mathematics stuff it doesn't mead you can remove it from language and document its bugs.

EDIT: After checking git blame, I see that this is probably a bug, after all. See the next post.

@tsbockman
Copy link
Contributor

@denis-sh @ibuclaw
I checked git blame, to see if the conflicting documentation and implementation were introduced by the same person, and they were.

The original implementation just called roundl; the more explicit CRuntime_Microsoft path was a later addition by a different person. My best guess is that the original author didn't check the documentation for roundl, and thought it did banker's rounding.

So, the bug is with the implementation, not the docs. Unfortunately, I see no good solution to this given that the mismatch has been in place for eight years now. The best thing would probably be to deprecate this function and tell people to use nearbyint or rint with the desired rounding mode set.

Just fixing the implementation after all this time could easily break just as much code as it fixed.

@denis-sh
Copy link
Contributor

denis-sh commented Dec 6, 2015

Yes, as we probably shouldn't just change functionality of existing functions it will be good to deprecate them to not pollute our library with useless C function analogs.
I'm for deprecating round and lround as they are just specializations of rint/lrint with rarely needed rounding mode.

@tsbockman
Copy link
Contributor

In the meantime, this PR should stay merged since we're not changing the implementation. A follow-up PR to deprecate round would be good, though.

I don't see a need to deprecate lround, since its documentation has matched its implementation from the beginning. (Not that I have much use for it, personally.)

@denis-sh
Copy link
Contributor

denis-sh commented Dec 6, 2015

I don't see a need to deprecate lround, since its documentation has matched its implementation from the beginning. (Not that I have much use for it, personally.)

This function is as useless as round. The main issue isn't in documentation, it is in usability and having functions nobody needs.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 6, 2015

to not pollute our library with useless C function analogs.

The intrinsics in std.math are no different to this either, to be honest. :-)

@tsbockman
Copy link
Contributor

The main issue isn't in documentation, it is in usability and having functions nobody needs.

The cost of maintaining a one-liner function like that is trivial, and there are no doubt people who are using it in their projects.

I do agree that std.math is a mess. However, I think rather than a constant churn of small, randomly distributed API improvements, it would be better to just replace the whole module with an std.math2.

Some of the changes I'd like to see (in no particular order):

  • Consolidate redundant functionality.
  • Consistent naming.
  • 64-bit double implementations available for all floating-point operations.
  • Actual usable and testable support for 128-bit types (integer and floating-point).
  • Eliminate unnecessary use of C library calls and assembly code.
  • Homogenize integer and floating-point operations.
  • Eliminate all intentional "divide by zero" errors. (Who's idea was it to make exceptions which are invisible to catch and nothrow part of the official API???)
  • All functions should work in CTFE.
  • Better template constraints.

I think fixing as much as possible of this stuff at once would be easier and less disruptive in the long run - especially since the old module could be kept for backwards compatibility as long as necessary.

@denis-sh
Copy link
Contributor

denis-sh commented Dec 6, 2015

I think fixing as much as possible of this stuff at once would be easier and less disruptive in the long run - especially since the old module could be kept for backwards compatibility as long as necessary.

Agree. Is there an enhancement request for this? It not, please file one in bugzilla to consolidate links and thoughts.

@tsbockman
Copy link
Contributor

@denis-sh

Is there an enhancement request for this?

This is probably into DIP territory, although my thought was to start by just writing something and sticking it on dub as a discussion starter.

Either way, my main focus right now is getting a checked integer module into Phobos. I might work on std.math2 after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants