On Thu, 4 Jan 2024 15:34:03 GMT, Jason Mehrens <d...@openjdk.org> wrote:
>>> > I think this ticket should focus on adding the new constructor as part of >>> > the API. >>> >>> Okay. I would think the code would avoid heapify when the caller does >>> foolish things with this API such as: >>> >>> ``` >>> SortedSet ss = filledSortedSet(); >>> PriorityQueue pq1 = new PriorityQueue(ss.comparator(), ss); >>> >>> PriorityQueue pq = filledPriorityQueue(); >>> PriorityQueue pq2 = new PriorityQueue(pq.comparator(), pq); >>> ``` >>> >>> I assume this constructor is going to be added to TreeSet, >>> PriorityBlockingQueue, and ConcurrentSkipListSet? >> >> why do you think the code would avoid heapify? `initFromCollection` method >> will be called regardless of the type of the collection passed, which will >> heapify the queue. >> >> regarding adding the constructor to the other types mentioned, I believe I >> can be done, probably as part of a different ticket. > >> why do you think the code would avoid heapify? `initFromCollection` method >> will be called regardless of the type of the collection passed, which will >> heapify the queue. > > I simply mean to point out: > > 1. That if the given comparator and the sortedset/priority queue comparator > are the same then the call to heapify should be avoided as this is what is > done in the [copy > constructor.](https://github.com/openjdk/jdk/blob/139abc453f9eeeb4763076298ec44573ab94a3b6/src/java.base/share/classes/java/util/PriorityQueue.java#L196) > 2. This new API has an anti-pattern not present with the other constructors. I think it's that @jmehrens arguing that the "set of constructors" should be settled and there shouldn't be more or less, like > This new API has an anti-pattern not present with the other constructors. In all honesty, our collection API docs is massively outdated, that we haven't even documented SequencedCollection since JDK 21; those "constructor parity" arguments are relics of the age of serialization and can be safely discarded. > I assume this constructor is going to be added to TreeSet, > PriorityBlockingQueue, and ConcurrentSkipListSet? Feel free to. If you submit a patch, we are more than glad to review it. An overhaul ("anti-pattern" in @jmehrens' words) caused by doing the right thing is not a reason not to do so. ----------------- @archiecobbs You are right. I will create a CSR for this patch. Hope the original author can come back (just issue `/open` in a comment and bot will reopen PR), otherwise we can probably add such comparator+copy constructors to all these ordered collections. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-2234768805
