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
JDK-8282686: Add constructors taking a cause to SocketException #7705
Conversation
|
/csr |
@jddarcy this pull request will not be integrated until the CSR request JDK-8282688 for issue JDK-8282686 has been approved. |
Webrevs
|
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.
This looks good, I just wonder if we should add a test for the new constructors. One of them will be tested by the usages in the JDK, the cause-only constructor may not be exercised by any code.
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.
Looks good to me. I may file a new RFE and use this constructor in the JSSE implementation.
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.
Looks fine, would be worth including a couple of tests for coverage
@jddarcy This change now passes all automated pre-integration checks. 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 4 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
|
/integrate |
Going to push as commit 1faa5c8.
Your commit was automatically rebased without conflicts. |
import java.net.SocketException; | ||
import java.util.Objects; | ||
|
||
public class TestSocketExceptionCtor { |
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 don't think this is a good name for the test because it tests mores than the constructors. So I think drop the suffix from the name.
Hmm, this seems to have been integrated without any Reviewer on the final commit. How did that happen? |
Please review this small API enhancement to add the usual constructors taking a cause to SocketException and then update uses of initiCause on creating SocketException to instead pass the cause via the constructor.
Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282688
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7705/head:pull/7705
$ git checkout pull/7705
Update a local copy of the PR:
$ git checkout pull/7705
$ git pull https://git.openjdk.java.net/jdk pull/7705/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7705
View PR using the GUI difftool:
$ git pr show -t 7705
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7705.diff