On Sat, 22 Apr 2023 18:40:45 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Add a method `internalName` to `ClassDesc`, and unifies handling of string >> representation of a class constant in CONSTANT_Class_info via >> `ofInternalName` and `internalName` APIs, documented in `ClassDesc` itself. >> In particular, `ofInternalName` now accepts arrays. >> >> The motivation of this API is that avoiding frequent String creations via >> caching (enabled by this new API, will be in a separate patch) would speed >> up Classfile API's [writing of simple class >> files](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java) >> by 1/3. See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-April/000296.html >> for more context. >> >> This API is futureproof: for Valhalla's Q-types, it will return their string >> representation in CONSTANT_Class_info, which is most likely their full >> descriptor string. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306697/java.base/java/lang/constant/ClassDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Unify ofInternalName and internalName, document about CONSTANT_Class_info, > remove misleading JVMS 4.4.1 links For API design, I have 3 choices for the accessor method: 1. Return Optional<String> only for internal names for classes or interfaces; Optional.empty() for primitives and arrays 2. Return Optional<String> suitable for CONSTANT_Class_info; Optional.empty() for primitives 3. Return String suitable for CONSTANT_Class_info; throw for primitives Which route should I take? I personally reject 1 as internal names are meaningless if they are used alone without being part of CONSTANT_Class_info. For 2 and 3, the main problem is the overhead of Optional vs whether there are better ways to detect if a class cannot be encoded in CONSTANT_Class_info (for now it's only primitives, but valhalla might change that if non-null types are loadable but cannot be in CONSTANT_Class_info) On second thought, I think I will go option 2; it will be in line with `Class.describeConstable()`; though the current check is based on `Class.isHidden()`, criteria may change when Valhalla is out, and `describeConstable()` is better suited for future API evolutions. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13598#issuecomment-1526055365
