RFR: 8375285: Port fdlibm asinh to Java [v7]
Anton Artemov
aartemov at openjdk.org
Wed Feb 4 11:59:43 UTC 2026
On Wed, 4 Feb 2026 10:50:00 GMT, Raffaello Giulietti <rgiulietti at openjdk.org> wrote:
>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>>
>> 8375285: Addressed reviewer's comments.
>
> src/java.base/share/classes/java/lang/FdLibm.java line 3514:
>
>> 3512: /**
>> 3513: * Return the Inverse Hyperbolic Sine of x
>> 3514: *
>
> This is inside a javadoc comment.
> If you want structuring into paragraphs, you need to add `<p>` at the beginning of new paragraphs, except the 1st.
>
> Alternatively, you could convert to a block comment. In a block comment you can use `<` rather than `<`, but then all the `&...;` HTML named entities make no much sense.
I do not see any other method in FdLibm.java having this type paragraph formatting in place. I will keep it as it is.
> src/java.base/share/classes/java/lang/FdLibm.java line 3520:
>
>> 3518: * asinh(x) is defined so that asinh(sinh(alpha)) = alpha, -∞ < alpha < ∞
>> 3519: * and sinh(asinh(x)) = x, -∞ < x < ∞
>> 3520: * It can be written as asinh(x) = ln(|x| + sqrt(x^2 + 1)), -∞ < x < ∞
>
> Suggestion:
>
> * It can be written as asinh(x) = sign(x) * ln(|x| + sqrt(x^2 + 1)), -∞ < x < ∞
Thanks for spotting.
> src/java.base/share/classes/java/lang/FdLibm.java line 3547:
>
>> 3545: return x + x; // x is inf or NaN
>> 3546: }
>> 3547: if (ix < 0x3e300000) { // |x| < 2**-28
>
> Suggestion:
>
> if (ix < 0x3e30_0000) { // |x| < 2**-28
Yes, missed that one.
> src/java.base/share/classes/java/lang/Math.java line 2763:
>
>> 2761: /**
>> 2762: * Returns the inverse hyperbolic sine of a {@code double} value.
>> 2763: * The inverse hyperbolic sine of <i>x</i> is defined to be a function such that
>
> Suggestion:
>
> * The inverse hyperbolic sine of <i>x</i> is defined to be the function such that
>
> Using "the" emphasises that the function is unique.
Addressed.
> src/java.base/share/classes/java/lang/Math.java line 2765:
>
>> 2763: * The inverse hyperbolic sine of <i>x</i> is defined to be a function such that
>> 2764: * asinh({@linkplain Math#sinh sinh(<i>x</i>)}) = <i>x</i> for any <i>x</i>.
>> 2765: * Note that both range and domain of the exact asinh are unrestricted.
>
> Suggestion:
>
> * Note that both domain and range of the exact asinh are unrestricted.
>
> I think that putting the domain first is more important for usage.
Yes, makes sense, addressed.
> src/java.base/share/classes/java/lang/Math.java line 2773:
>
>> 2771: *
>> 2772: * <li>If the argument is infinity, then the result is
>> 2773: * negative infinity with the same sign as the argument.
>
> Suggestion:
>
> * infinity with the same sign as the argument.
That is a typo, thanks for spotting.
> src/java.base/share/classes/java/lang/Math.java line 2777:
>
>> 2775: * <li>If the argument is NaN, then the result is NaN.
>> 2776: *
>> 2777: * <p>The computed result must be within 2.5 ulps of the exact result.
>
> This paragraph should come _after_ the bulleted list.
Moved out as suggested.
> src/java.base/share/classes/java/lang/StrictMath.java line 2194:
>
>> 2192: * @return The inverse hyperbolic sine of {@code x}.
>> 2193: * @since 27
>> 2194: */
>
> Same comments as for Math.
Addressed.
> test/jdk/java/lang/Math/HyperbolicTests.java line 1413:
>
>> 1411: * Therefore,
>> 1412: *
>> 1413: * 1. For large values of x asinh(x) ~= signum(x)
>
> I don't get this.
> signum() is either 1, -1, or 0.
There is nothing wrong with the approx equality itself, but the whole statement is wrong, as the range is unrestricted. I removed this line.
> test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 2761:
>
>> 2759:
>> 2760: /**
>> 2761: * Return the Inverse Hyperbolic Sine of x
>
> I suggest turning this into a block comment, and adjust the indentation by one space in the lines below.
I noticed that some methods in this file have doc-style comments, some have block comments. Addressed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763627144
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763627564
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763627901
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763628176
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763628507
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763628976
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763629790
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763630112
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763630703
PR Review Comment: https://git.openjdk.org/jdk/pull/29273#discussion_r2763631265
More information about the core-libs-dev
mailing list