-
Notifications
You must be signed in to change notification settings - Fork 146
[type-classes] 8376040: Add prototype "textbook complex" class #1942
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
Conversation
|
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
|
@jddarcy This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
Since the HTML markup in the javadoc for the arithmetic methods is hard to read, here is a copy and paste of the rendered text: public static ComplexTextbook add(ComplexTextbook addend, ComplexTextbook augend) Addition operation, binary "+". Implementation Requirements: public static ComplexTextbook subtract(ComplexTextbook minuend, ComplexTextbook subtrahend) Subtraction operation, binary "-". Implementation Requirements: public static ComplexTextbook multiply(ComplexTextbook multiplier, ComplexTextbook multiplicand) Multiplication operation, "*". API Note: public static ComplexTextbook divide(ComplexTextbook dividend, ComplexTextbook divisor) Division operation, "/". API Note: |
Webrevs
|
| if (that instanceof ComplexTextbook c) { | ||
| return this.real == c.real && this.imag == c.imag; | ||
| } else { | ||
| return false; | ||
| } |
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.
| if (that instanceof ComplexTextbook c) { | |
| return this.real == c.real && this.imag == c.imag; | |
| } else { | |
| return false; | |
| } | |
| return that instanceof ComplexTextbook c && real == c.real && imag == c.imag; |
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.
Done; I'll make any other necessary adjustments to the java.lang.Object methods when adding support for overloaded operators.
| double a = c.real; | ||
| double b = c.imag; | ||
| return StrictMath.hypot(a, b); | ||
| } |
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.
Since we have abs(), is there a reason for not having theta() (or arg(), or whatever)?
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.
Those could be added in a subsequent iteration, sure.
| */ | ||
| public double imag() { // better as a static method? | ||
| return imag; | ||
| } |
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.
Besides being shorter and unambiguous, re() and im() are the standard names used in maths AFAIK.
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.
Hmm.
I'd prefer to leave the more idiomatic or Java "real" and "imag" for now, but acknowledged "re" and "im" are common in other contexts.
|
/integrate |
|
Going to push as commit da76b22.
Your commit was automatically rebased without conflicts. |
First cut at a version of complex numbers using the textbook algorithms for the arithmetic operations. This version is intended for prototyping and not for production use.
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1942/head:pull/1942$ git checkout pull/1942Update a local copy of the PR:
$ git checkout pull/1942$ git pull https://git.openjdk.org/valhalla.git pull/1942/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1942View PR using the GUI difftool:
$ git pr show -t 1942Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1942.diff
Using Webrev
Link to Webrev Comment