RFR 8209138: Symbol constructor uses u1 as the element type of its name argument

[David Holmes]

Hi Harold,

On 21/08/2018 12:43 AM, Harold David Seigel wrote:
> Hi,
> 
> Please review this change for bug JDK-8209138.  The fix changes class 
> Symbol in symbol.hpp to use type char instead of types u1 and jbyte and 
> renames relevant functions by replacing 'byte' with 'char'.  For 
> example, 'Symbol::byte_at_put()' is now 'Symbol::char_at_put()'.

It's not clear to me exactly what all these things should be. In a lot 
of places we seem to be dealing with UTF8 character representations, 
which to me should be u1 (afterall that is how they are specified in the 
classfile format!) which is just an unsigned "byte". But char is signed 
(in our build).

The current change just seems to push out the boundary where we convert 
between things. For example ciSymbol now has a char base()** but still 
has a byte_at() method - should that not know be char_at() for 
consistency? Especially given byte_at() returns get_symbol()->char-at()

** Granted the existing jbyte seems the wrong choice too.

I think we have a lot of type confusion in our code, but it isn't clear 
to me that this particular change is necessarily changing things in the 
right direction.

Cheers,
David


> Open Webrev: 
> http://cr.openjdk.java.net/~hseigel/bug_8209138/webrev/index.html
> 
> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8209138
> 
> The change was tested by running Mach5 tiers 1 and 2 tests and builds on 
> Linux-x64, Windows, and Mac OS X, running tiers 3-5 tests on Linux-x64, 
> and by running JCK-11 API, Lang and VM tests on Linux-x64.
> 
> Thanks, Harold
>