👋 Welcome back mkarg! 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.
❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.
/csr
@mkarg has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mkarg please create a CSR request for issue JDK-8343110 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
335 | * | ||
336 | * @since 24 | ||
337 | */ | ||
338 | public default void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) { |
Shouldn't the getChars
methods of String
, AbstractStringBuilder
, StringBuilder
and StringBuffer
be marked with @Override
?
While technically not being necessary, it is definitively a good idea. I will add it to the draft once we actually agreed that we really want to go with this particular signature (see the discussion with Chen).
Sorry for belated mail response, but I think we should design the API to not take source start/end. I think JIT can escape analysis the new String in practice.
Sorry for belated mail response, but I think we should design the API to not take source start/end. I think JIT can escape analysis the new String in practice.
Chen, thank you for chiming in! There is nothing to be sorry, I was just posting drafts so far! 🙂
Regarding your actual question, I do understand your idea and while originally I had the same in mind (it really is appealing!), I came up with a draft using the original String.getChars()
signature instead, due to the following drawbacks:
CharSequence.getChars(int, int, char[], int)
implementations already, as this problem (and the idea how to solve it) is anything but new. At least such implementations are String
, StringBuilder
and StringBuffer
. If we come up with a different signature, then none of these already existing performance boosters will get used by Reader.of(CharSequence)
automatically - at least until they come up with alias methods. Effectively this leads to (possibly lots) of alias methods. At least it leads to alias methods in String
, StringBuilder
, StringBuffer
and CharBuffer
. In contrast, when keeping the signature copied from String.getChars
, chances are good that (possibly lots of) implementations will instantly be supported by Reader.of(CharSequence)
without alias methods. At least, String
, StringBuilder
and StringBuffer
will be.StringBuilder.getChars(int, int, char[], int)
, so (possibly a lot of) people might simply expect us to come up with that lengthy signature. These people might be rather confused (if not to say frustrated) when we now force them to write an intermediate subSequence(int, int)
for something that was "such simple" before.CharSequence.subSequence
could come up with the (performance-wise "bad") idea of creating copies instead of views. At least it seems like AbstractStringBuilder
is doing that, so chances are "good" that custom libs will do that, too. For example, because they need it for safety. Or possibly, because they have a technical reason that enforces a copy. That would (possibly massively, depending on the actual class) spoil the idea of performance-boosting this PR is actually all about.Hi Markus, I recommend to continue the discussion on the mailing list instead of on GitHub. Note that GitHub pull requests are only forwarded to the mailing list when it's ready for review, and this proposal is way too early. (And the CSR is too early, too: we should agree on an API surface, such as exception contracts, first)
Sorry that no one has responded to you yet, but many engineers are busy with other areas, such as pushing the JEPs (as you see, we have a huge number of candidate/submitted JEPs right now, and JDK 24 RDP1 is just 6 weeks away). They are monitoring your core-libs thread, stay assured!
@liach Agreed with all you say, but actually: I did not expect any response - in particular on a weekend!
I do not know why apparently people started to review the draft documents. I just filed it for no other purpose than taking note of its existence. 🤔 I thought it is pretty clear to everybody that draft means "do not review". At least this is how I used draft documents in the past 30 years. 🙂
So it seems there was a misunderstanding here regarding my drafts. My intention ever was and still is to first finish the discussion about alternatives (A)...(E) on the core-lib mailing list first. My PR and CSR drafts are intentionally in draft state, which means, they are not open for review yet (there is no rfr
label) and solely serve as my public storage for ideas-in-progress in lieu of out-of-band media. The detail discussion you started regarding details of the PR was a bit too early IMHO (and surprised me, and actually your duplicate question in the PR confused me), as it is not even decided that we actually go with proposal A, so why discussing details of exactly that already (my conclusion was, you like to prepare it for the case it becomes the finalist of the mailing list discussion)? Maybe I misunderstood your intent, and you actually wanted to propose alternative (F), then sorry for me not seeing that in the first place.
@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!
Kindly asking anybody's contribution to the ongoing discussion on the core-devs mailing list. Thanks you.
Login to write a write a comment.
This Pull Request proposes an implementation for [JIRA]: Adding the new method
public void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin)
to theCharSequence
interface, providing a bulk-read facility including a default implementation iterating overcharAt(int)
.In addition, this Pull Request proposes to replace the implementation of
Reader.of(CharSequence).read(char[] cbuf, int off, int len)
to invokeCharSequence.getChars(next, next + n, cbuf, off)
instead of utilizing pattern matching for switch. Also, this PR proposes to implementCharBuffer.getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin)
as an alias forCharBuffer.get(srcBegin, dst, dstBegin, srcEnd - srcBegin)
.To ensure quality...
AbstractStringBuilder.getChars(...)
.Reader.of(CharSequence)
, as these provide sufficient coverage of all changes introduced by this PR.Progress
Issues
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21730/head:pull/21730
$ git checkout pull/21730
Update a local copy of the PR:
$ git checkout pull/21730
$ git pull https://git.openjdk.org/jdk.git pull/21730/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21730
View PR using the GUI difftool:
$ git pr show -t 21730
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21730.diff