My first webrev review request: 6850337 Hasher.java interprets given option value badly

[Xueming Shen]

Ulf,

I believe the "mb" is designed to be "exclusive". I'm the one to blame, 
I changed the
default mb value from 10 to 11 for my 1.6 Console work, which moved all OEM
charsets from the "extended" into the "standard" therefor required a 
bigger default
value, but I FORGOT to update the help message as well. My bad. I guess the
better fix for this one is to update the err.println(...). I will be the 
sponsor if you still
want to "fix" this one.

Sherman

btw,
(1) The bugid# which I updated the mb from 10 to 11 is #6349456.
(2) So far the "convention" is to put the contributor name into the hg 
commit message not
the source file.

Ulf Zibis wrote:
>
>
>> Are you sure this is not done by design and that mb is not in fact
>> meant to be an exclusive rather than inclusive bound?
>>   
>
> Option usage of Hasher.java says:
> "    -md depth    max chain depth (default 3)"
> "    -mb bits     max index bits (lg of table size, default 10)"
> In case of setting -md to x the max chain depth properly results in x, 
> but ...
> in case of setting -mb to x the max lg of table size inproperly 
> results in x-1 bits, as you can see in -verbose output of the hasher.
>
> To ensure, that the default value is processed as it would be 10, 
> variable maxBits is pre-set to 11, to "workaround" the bug.
> When fixing this bug, any usage of this option should be corrected to 
> x-1 to ensure identical results.
>
> I don't think the different boundary behaviour of -md and -mb is done 
> by design. Maybe processing of -md has been wrong too in first time 
> and was corrected, but correction for -mb was overseen, as never used.
>
> Maybe you could have a short call to the author Mark Reynolds to 
> ensure this. Possibly he also has knowledge of other usage than in 
> genCharsetProvider.sh.
>
>> Have you checked that the OpenJDK can still be built with this change 
>> in place?
>>   
>
> I have scanned over
> - hg.openjdk.java.net/jdk7/tl/jdk/make
> - hg.openjdk.java.net/jdk7/tl/jdk/src
> - hg.openjdk.java.net/jdk7/tl/jdk/test
>
> The only usage of Hasher.java I've found in:
> - hg.openjdk.java.net/jdk7/tl/jdk/make/java/nio/genCharsetProvider.sh
>
> In this case, the option -mb  is not used. I assume, this is the 
> reason, why nobody has experienced this bug before.
>
> I have *not* scanned over
> - hg.openjdk.java.net/jdk7/tl/hotspot .../langtools etc.
> as I don't have a hg clone of them.
>
> Maybe I should have done this, but I think, it's anyway good that 
> experienced SUN engineer would do that scan too, and I guess, you have 
> better tools to do that.
>
> -Ulf
>
>