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
7113208: Incorrect javadoc on java.net.DatagramPacket.setLength() #10037
Conversation
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
Webrevs
|
* will be used for receiving data. The {@code length} plus the | ||
* {@link #getOffset() offset} must be lesser or equal to the | ||
* length of the packet's data buffer. |
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.
The new text looks good to me. While you're at it I would suggest to rewrite the code that checks the length with something like:
Preconditions.checkFromIndexSize(offset, length, buf.length,
Preconditions.outOfBoundsExceptionFormatter(IllegalArgumentException::new));
Then figure out if we have a unit test that already checks this, and if not, write one!
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.
Hello Daniel, thank you for the review. I've updated the PR to implement the code change you suggested.
There's already test/jdk/java/net/DatagramPacket/Setters.java
which has testSetLength
to test this method. I've run the entire test/jdk/java/net/DatagramPacket
tests with this change and they passed fine. I'll trigger a more comprehensive test run just to make sure nothing unexpected shows up.
Preconditions.checkFromIndexSize(offset, length, buf.length, | ||
Preconditions.outOfBoundsExceptionFormatter(IllegalArgumentException::new)); |
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 for that. Maybe the bound checks in
public synchronized void setData(byte[] buf, int offset, int length) {
could be replaced in a similar way.
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.
Hello Daniel,
I've now updated this method too to use Preconditions
. Additionally, a NullPointerException
check which was implicit in this method (and noted by a code comment) has now been made an explicit check (using Objects.requireNonNull
) in this updated version. I hope that's OK.
The existing Setters.java
test case covers this method as well and I've run it after these changes to make sure it continues to pass.
* equal to the offset plus the length of the packet's buffer. | ||
* will be used for receiving data. The {@code length} plus the | ||
* {@link #getOffset() offset} must be lesser or equal to the | ||
* length of the packet's data buffer. |
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.
The javadoc update looks okay.
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 Jaikiran! LGTM!
@jaikiran 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 88 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thank you Alan and Daniel for the reviews. tier1,tier2,tier3 testing went fine with these changes. |
/integrate |
Going to push as commit a366e82.
Your commit was automatically rebased without conflicts. |
Can I please get a review of this javadoc only change for
DatagramPacket#setLength()
method? This addresses https://bugs.openjdk.org/browse/JDK-7113208.I haven't create a CSR because the javadoc was already stating the correct behaviour in the
@throws
documentation as follows:This commit merely fixes the main part of the javadoc.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10037/head:pull/10037
$ git checkout pull/10037
Update a local copy of the PR:
$ git checkout pull/10037
$ git pull https://git.openjdk.org/jdk pull/10037/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10037
View PR using the GUI difftool:
$ git pr show -t 10037
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10037.diff