Hi Roger,

On 12/11/18 10:12 AM, Roger Riggs wrote:
Hi Naoto,

Looks ok,

I intended only the number of elements to be defined as a constant.
The other factors can be hard coded.

OK, I revised it again:

http://cr.openjdk.java.net/~naoto/8215194/webrev.02/


In the test, you will still have to edit the test when the number changes.
I meant to avoid that edit.  Though then may be there is not need for the test at all.

Yes, if we just reflectively use the constant in Character.UnicodeBlock in the test, it is guaranteed to succeed no matter what. Thus I added the assertion. Still, the test ofHashMap() succeeds till the block additions surpasses that 1024 capacity (thus this was overlooked on Unicode 11 upgrade).

Naoto


Roger


On 12/11/2018 12:59 PM, Naoto Sato wrote:
Hi Roger,

Thanks. I updated it as suggested (incl. test using reflection):

http://cr.openjdk.java.net/~naoto/8215194/webrev.01/

Naoto

On 12/11/18 7:57 AM, Roger Riggs wrote:
Hi Naoto,

Since the value changes from time to time, it would give it some visibility
if it were defined using a private final int  (or float)
      private final int MAP_CAPACITY = 667;

Though I suppose the test can't use the value without using reflection.
But it would lower the maintenance in the long term.

$.02, Roger

On 12/11/2018 09:51 AM, Naoto Sato wrote:
Hi,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8215194

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8215194/webrev.00/

This one line fix is for the correctness of the initial map size of Character.UnicodeBlock.

Naoto


Reply via email to