-
Notifications
You must be signed in to change notification settings - Fork 738
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
ext/bcmath - Added bcceil
, bcfloor
, bcround
and bcdivmod
#4132
base: master
Are you sure you want to change the base?
Conversation
ccfc782
to
a08ba3e
Compare
a08ba3e
to
4ebd9d4
Compare
4ebd9d4
to
9110b52
Compare
bcceil
, bcfloor
, bcround
and bcdivmod
9110b52
to
00ac270
Compare
I will wait for reviewing this when the other 2 PRs are updated based on my review comments, as some similar markup mistakes are in this PR. |
done |
Oh, no, there's more |
done! |
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
fixed! |
reference/bc/functions/bcceil.xml
Outdated
<refentry xml:id="function.bcceil" xmlns="http://docbook.org/ns/docbook"> | ||
<refnamediv> | ||
<refname>bcceil</refname> | ||
<refpurpose>Round arbitrary precision number fractions up</refpurpose> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the description very weird sounding, this doesn't really read like a proper sentence. Similar for the other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit) How about 'Round up the fractional part of an arbitrary precision number' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just "Round up arbitrary precision number" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed in 3d93dce
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this is fine now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment about showing the output of examples, but otherwise LGTM
reference/bc/functions/bcfloor.xml
Outdated
<programlisting role="php"> | ||
<![CDATA[ | ||
<?php | ||
echo bcfloor('4.3'); // '4' | ||
echo bcfloor('9.999'); // '9' | ||
echo bcfloor('-3.14'); // '-4' | ||
?> | ||
]]> | ||
</programlisting> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use var_dump()
and show the result of the example via a <screen>
tag rather than a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed it in 501f4d9
RFC:
https://wiki.php.net/rfc/adding_bcround_bcfloor_bcceil_to_bcmath
https://wiki.php.net/rfc/add_bcdivmod_to_bcmath
I found an error in the existing documentation regarding
bcdiv
andbcmod
.But instead of this PR, I'll follow it up with another PR.
Regarding errors/exceptions, I will add everything except
bcdivmod
later.