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

Add missing bounds check to deleteCharAt #15979

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

mikezhang1234567890
Copy link
Contributor

@mikezhang1234567890 mikezhang1234567890 commented Sep 27, 2022

For StringBuilder and StringBuffer
Add tests for deleteCharAt with out of bound indices.

Fixes #15897

Tested internally here running JCL_Test_SE80

@pshipton
Copy link
Member

jenkins test sanity,extended,sanity.openjdk alinux64 jdk8

@keithc-ca
Copy link
Contributor

delete(int start, int end) doesn't appear to match its documented behavior; perhaps all that is needed (e.g. in StringBuffer) is to remove this:

    if (end > currentLength) {
        end = currentLength;
    }

@mikezhang1234567890
Copy link
Contributor Author

mikezhang1234567890 commented Sep 27, 2022

In this case, I think our doc is wrong, last condition to throw the exception should be start > length() instead of end > length().

@pshipton
Copy link
Member

@keithc-ca
Copy link
Contributor

In this case, I think our doc is wrong, last condition to throw the exception should be start > length() instead of end > length().

I think the documentation is correct. It might be better written as three {@code} blocks instead of two.

For StringBuilder and StringBuffer
Add tests for deleteCharAt with out of bound indices.

Signed-off-by: Mike Zhang <mike.h.zhang@ibm.com>
@mikezhang1234567890
Copy link
Contributor Author

Updated the docs for delete as well to hopefully make things clearer. Can you take another look please @keithc-ca ?

@keithc-ca
Copy link
Contributor

jenkins test sanity alinux64 jdk8

@pshipton
Copy link
Member

pshipton commented Sep 27, 2022

I'm inclined to include this in the 0.35 release since it fixes a bug / user problem, the fix appears low risk, and we haven't yet done the M2 build for jdk8. @keithc-ca thoughts?

@keithc-ca
Copy link
Contributor

I'm inclined to include this in the 0.35 release since it fixes a bug / user problem, the fix appears low risk, and we haven't yet done the M2 build for jdk8.

I agree.

@pshipton
Copy link
Member

@mikezhang1234567890 please create a PR against the 0.35 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants