RFR 8209138: Symbol constructor uses u1 as the element type of its name argument
kim.barrett at oracle.com
Tue Aug 21 05:19:51 UTC 2018
> On Aug 20, 2018, at 8:53 PM, David Holmes <david.holmes at oracle.com> wrote:
> Hi Harold,
> On 21/08/2018 12:43 AM, Harold David Seigel wrote:
>> 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).
Not sure what’s meant by “in our build” but char is unsigned when building HotSpot for aarch64 / arm. See, for example, JDK-8209586.
> 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.
>> 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
More information about the hotspot-runtime-dev