Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

ext/bcmath: BcMath\Number class #4187

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

SakiTakamachi
Copy link
Member

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Partial review as there is already a bunch that needs fixing across the board.

reference/bc/bcmath.number.xml Outdated Show resolved Hide resolved
reference/bc/bcmath.number.xml Outdated Show resolved Hide resolved
reference/bc/bcmath.number.xml Outdated Show resolved Hide resolved
reference/bc/bcmath.number.xml Outdated Show resolved Hide resolved
reference/bc/bcmath.number.xml Outdated Show resolved Hide resolved
reference/bc/bcmath/number/construct.xml Outdated Show resolved Hide resolved
reference/bc/bcmath/number/add.xml Outdated Show resolved Hide resolved
reference/bc/bcmath/number/add.xml Outdated Show resolved Hide resolved
reference/bc/bcmath/number/tostring.xml Outdated Show resolved Hide resolved
reference/bc/bcmath/number/sub.xml Outdated Show resolved Hide resolved
SakiTakamachi and others added 7 commits December 2, 2024 12:08
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Gina Peter Banyard <girgias@php.net>
@SakiTakamachi
Copy link
Member Author

@Girgias
Fixed!

@SakiTakamachi SakiTakamachi requested a review from Girgias December 2, 2024 04:16
@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Dec 2, 2024

Hmm. I don't understand why CI turns red...
No problems occur in my local

edit:
I wanted to change the explanation a little, so I stopped including on the part where the error occurred.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Some comments, the only concern is with the XIncludes, where I think waiting for @alfsb's PR to be merged might make more sense.

Comment on lines +2158 to +2161
<!ENTITY bc.number.scale.description '<varlistentry xmlns="http://docbook.org/ns/docbook"><term>
<parameter>scale</parameter></term><listitem><simpara><parameter>scale</parameter> explicitly
specified for calculation results. If &null;, the <parameter>scale</parameter> of the calculation
result will be set automatically.</simpara></listitem></varlistentry>'>
Copy link
Member

Choose a reason for hiding this comment

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

No need to put this on a single line.

Suggested change
<!ENTITY bc.number.scale.description '<varlistentry xmlns="http://docbook.org/ns/docbook"><term>
<parameter>scale</parameter></term><listitem><simpara><parameter>scale</parameter> explicitly
specified for calculation results. If &null;, the <parameter>scale</parameter> of the calculation
result will be set automatically.</simpara></listitem></varlistentry>'>
<!ENTITY bc.number.scale.description '<varlistentry xmlns="http://docbook.org/ns/docbook">
<term>
<parameter>scale</parameter>
</term>
<listitem>
<simpara>
<parameter>scale</parameter> explicitly specified for calculation results.
If &null;, the <parameter>scale</parameter> of the calculation result will be set automatically.
</simpara>
</listitem>
</varlistentry>'>

Comment on lines +20 to +21
This class is not affected by <link linkend="ini.bcmath.scale">bcmath.scale</link>
in &php.ini;.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This class is not affected by <link linkend="ini.bcmath.scale">bcmath.scale</link>
in &php.ini;.
This class is not affected by the
<link linkend="ini.bcmath.scale">bcmath.scale</link>
INI directive set in &php.ini;.

<note>
<simpara>
The behavior of an overloaded operator is the same as specifying &null; for the
<parameter>scale</parameter> parameter in the corresponding method.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<parameter>scale</parameter> parameter in the corresponding method.
<parameter>scale</parameter> parameter on the corresponding method.

Comment on lines +86 to +87
For objects resulting from calculations, this value is set automatically unless explicitly
specify the <parameter>scale</parameter> parameter in the calculation method.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For objects resulting from calculations, this value is set automatically unless explicitly
specify the <parameter>scale</parameter> parameter in the calculation method.
For objects resulting from calculations, this value is automatically computed and set,
unless the <parameter>scale</parameter> parameter was set in the calculation method.

</methodsynopsis>
<simpara>
Compare two arbitrary precision numbers.
This method behaves similar to the spaceship operator.
Copy link
Member

Choose a reason for hiding this comment

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

Add a link to it?

Comment on lines +56 to +57
The <property>BcMath\Number::scale</property> will never be less than the <property>BcMath\Number::scale
</property> before expansion.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The <property>BcMath\Number::scale</property> will never be less than the <property>BcMath\Number::scale
</property> before expansion.
The <property>BcMath\Number::scale</property> will never be less than the
<property>BcMath\Number::scale</property> before expansion.

Comment on lines +58 to +61
</simpara>
<example>
<title>Example of <property>BcMath\Number::scale</property> of result object</title>
<programlisting role="php">
Copy link
Member

Choose a reason for hiding this comment

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

Move the example to the corresponding section, if you want to point it out in the return value you can add an xml:id and link tag

Comment on lines +40 to +43
<!-- ValueError cases -->
<xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('bcmath-number.add')/db:refsect1[@role='errors']/db:para[1])" />
<!-- The DivisionByZeroError case -->
<xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('bcmath-number.div')/db:refsect1[@role='errors']/db:simpara[1])" />
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to wait for php/doc-base#198 to be merged than using XIncludes with indices

<simpara>
The exponent. Must be a value with no fractional part.
The valid range of the <parameter>exponent</parameter> is platform specific,
but is at least <literal>-2147483648</literal> to <literal>2147483647</literal>.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
but is at least <literal>-2147483648</literal> to <literal>2147483647</literal>.
but it is at least <literal>-2147483648</literal> to <literal>2147483647</literal>.

<term><parameter>exponent</parameter></term>
<listitem>
<simpara>
The exponent, as an non-negative and integral (i.e. the scale has to be zero).
Copy link
Member

Choose a reason for hiding this comment

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

Use the same wording as in the pow method?

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.

2 participants