On Wed, 16 Feb 2022 19:11:53 GMT, XenoAmess <d...@openjdk.java.net> wrote:
>> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > fix test OK, good progress. Yes, leaving ConcurrentHashMap to another PR is fine. Do the changes to Class.java and the enum optimal capacity test need to be part of this PR? They seem unrelated. It's not clear to me that test is actually testing anything useful; it's just testing the same computation a couple different ways. The test seems reasonable enough and is a good start. There are a number of things that could be improved about it though. 1) It should probably be a TestNG test. This will allow you to use a DataProvider and also use TestNG assertions. 2) There are 12 test cases here, which seems amenable to using a DataProvider. You could try to make a little "combo" test that combines the three classes with the four ways of creating them, but it might be difficult to do that without using reflection. It might be easier to write a table with 12 cases, even if there is a bit of repetition. 3) Instead of writing reflective code to create the maps, it's probably easier to use suppliers that create the maps into the desired state. Each of the 12 test cases should have a lambda that does the creation. The test method then invokes the supplier and makes its assertions over the resulting map instance. 4) The `fill12` method can be used in an expression if it returns its argument. 5) Create a map with 12 entries as part of the test setup, and then just use it as the copy constructor argument. Note, I'll be on vacation next week, so there will be a gap in my responses during that time. test/jdk/java/util/Map/HashMapsPutAllOverAllocateTableTest.java line 2: > 1: /* > 2: * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. Fix copyright year. ------------- PR: https://git.openjdk.java.net/jdk/pull/7431
