On Oct 8, 2013, at 1:51 AM, Chris Hegarty wrote: > This looks much better. > > loadfactor and size validation look good, and in line with the original > suggestion in the bug. > > For the initial capacity what was originally suggested was to use a similar > technique to that of HashMap.readObject ( to ensure that the table never > needs to be rehashed during load, and ends up being at least 25% full). > Snippet from the bug: > > int capacity = (int)Math.min(size * Math.min(1 / loadFactor, 4.0f), > HashMap.MAXIMUM_CAPACITY); > > I'm curious why you have not taken that suggestion? What you have looks ok, > but it does reply on reasonable capacity and loadfactor values.
I saw that suggestion. I was trying insofar as possible to use the field values as serialized. After reading your comments above however I think that the suggestion is better. I have updated the webrev accordingly http://cr.openjdk.java.net/~bpb/8016252/webrev.3/ Note that this is a new URL. > I have not looked in any detail at the test, but the file should start with > the copyright/header, and not the import statements. Fixed - thanks. On Oct 8, 2013, at 2:14 AM, Alan Bateman wrote: > This looks better, the checks on loadFactor and size look okay. > > I don't know all the history on this bug but it looks like the goal was > partly to avoid resizing when deserializing. This means the capacity could > match HashMap. To be consistent with the size check then we could throw an > exception if the capacity is negative (interesting case for tests and malware > only). Negative capacity check added in the updated webrev.3. > I skimmed over the test but it doesn't appear to exercise anything new. If > you want to exercise the checks then it would require deserializing from a > byte stream that results in bad loadFactor, size and capacity values. It > might not be worth it of course. No, it does not exercise anything new. I actually did try inserting random garbage into the written-out byte stream, but without knowing where to do so to affect the fields of interest it is rather useless and causes totally unpredictable results. I don't know that this test really needs to be included with the patch. Thanks, Brian
