On Wed, 20 Dec 2023 05:05:09 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> @liach thanks for the help. I updated the PR title, also your proposed CSR >> content looks good to me. would you mind creating it with your proposed >> content? > > @valeh > > Hello, thanks for contributing this change. I can sponsor it. > > Changes to the JDK such as this one require tests to be included. You can see > some PriorityQueue tests in test/jdk/java/util/PriorityQueue. However, it > looks like these are tests for specific features and bugfixes. A > comprehensive set of tests resides in > test/jdk/java/util/concurrent/tck/PriorityQueueTest.java. It might be best to > add a test case for the new constructor there. > > In addition, changes such as this will require a release note. I've added the > `release-note=yes` label to the bug report to indicate this. The release note > process is documented in the Developer's Guide here: > > https://openjdk.org/guide/#release-notes > > @liach > > Thanks for starting the CSR draft. It looks straightforward enough so far; a > couple quick comments. 1) It's not necessary to include the implementation in > the CSR, just the specification (or specification changes or diffs) and the > method signature. 2) You might want to wait until the spec wording settles > down before creating the CSR, otherwise you'll have to keep them in sync > continually. > > -- > > In passing, I note this Stack Overflow answer in response to a question about > the lack of a PriorityQueue(Collection, Comparator) constructor: > > https://stackoverflow.com/questions/66170496/java-priority-queue-build-heap-process-time-complexity/66174529#66174529 > > In the discussion of this bug report, I think we believe that constructing a > PQ with the new constructor should be faster than constructing one with a > Comparator and then calling addAll(), but this SO answer seems to indicate > that this new constructor isn't necessary. I'm kind of skeptical of this, > though. It would be good to have some confirmation that the new constructor > indeed provides a performance improvement. Note however that benchmarking can > easily turn into a distraction and a time sink, so I don't think a benchmark > is required for this change. However, if somebody wants to try something out, > please do so. If suitable, a benchmark could be added somewhere in the > test/micro hierarchy (possibly as part of a different PR). > > Finally, please note that with the holiday season, I'm planning to take > several days away from work, so I might not respond quickly, but I eventually > will. @stuart-marks thanks for the suggestions, I also added the tests as you suggested ------------- PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1870705915
