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

Consolidate declarations of MAX_ARRAY_SIZE in one place #1506

Closed
wants to merge 8 commits into from

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Nov 29, 2020

In JDK there are multiple declarations of maximum array size (Integer.MAX_VALUE - 8) with exactly the same code and documentaion.
In some places, e.g. in AbstractStringBuilder they have no use in code, only in JavaDoc, in other ones they could be substituted with one single declaration. We already have one in jdk.internal.util.ArraysSupport and it can be propagated into java.util.Arrays and used from there (also making it available for JDK users).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1506/head:pull/1506
$ git checkout pull/1506

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 29, 2020

👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 29, 2020

@stsypanov The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-jfr
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@stsypanov stsypanov changed the title Consolidate declaration of MAX_ARRAY_SIZE in one place Consolidate declarations of MAX_ARRAY_SIZE in one place Nov 30, 2020
Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of the implementation limit for array sizes is not a specified fixed value and should not be public, it may need to change one day.

@stsypanov
Copy link
Contributor Author

@RogerRiggs thanks for pointing this out, I've made the field private

@stuart-marks
Copy link
Member

I'd suggest looking at #1617 . That PR not only renames this constant to be more descriptive; it also clarifies its semantics. After it's integrated, I think each of the places that has some constant MAX_ARRAY_SIZE or similar should be revisited and maybe changed to use either the constant, or the newLength method, or not changed at all. For example, the usage in InputStream is somewhat different. I don't see any reason that readNBytes should avoid allocating more than MAX_ARRAY_SIZE (or SOFT_MAX_ARRAY_LENGTH) if it had actually read that many bytes. On the other hand, a close look should be given to ArrayDeque, which can probably use newLength directly, since it has a very similar array growth policy as ArrayList.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 1, 2021

@stsypanov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 1, 2021

@stsypanov This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

@bridgekeeper bridgekeeper bot closed this Mar 1, 2021
@stsypanov stsypanov deleted the consolid branch September 20, 2021 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants