On Thu, 29 Apr 2021 04:31:35 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Hi Chris, >> >> Thanks for looking into this, >> >>>Do we even need findComponentType() any more? Isn't >>>ReferenceTypeImpl.findType() sufficient. >> >> We still need findComponentType(), >> Difference between findType() and findComponentType() is that, >> findComponentType() tries to get the list of ReferenceType from the >> "vm.classesByName". In case list is empty, it explicitly throws >> ClassNotLoadedException. >> This exception check is required in validateAssignment(ValueContainer >> destination) call from ObjectReferenceImpl.java. >> >> https://github.com/openjdk/jdk/blob/master/src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java#L579 >> >> >> >> I have modified the method a bit to handle that scenario >> >> >> diff --git a/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java >> b/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java >> index e544c81ae3e..54deba43894 100644 >> --- a/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java >> +++ b/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java >> @@ -95,20 +95,13 @@ public class ArrayTypeImpl extends ReferenceTypeImpl >> JNITypeParser sig = new JNITypeParser(signature); >> if (sig.isReference()) { >> // It's a reference type >> - JNITypeParser parser = new JNITypeParser(componentSignature()); >> - List<ReferenceType> list = vm.classesByName(parser.typeName()); >> - Iterator<ReferenceType> iter = list.iterator(); >> - while (iter.hasNext()) { >> - ReferenceType type = iter.next(); >> - ClassLoaderReference cl = type.classLoader(); >> - if ((cl == null)? >> - (classLoader() == null) : >> - (cl.equals(classLoader()))) { >> - return type; >> - } >> + try { >> + Type componentType = findType(componentSignature()); >> + return componentType; >> + // Component class has not yet been loaded >> + } catch (ClassNotLoadedException ex) { >> + throw new ClassNotLoadedException(componentTypeName()); >> } >> - // Component class has not yet been loaded >> - throw new ClassNotLoadedException(componentTypeName()); >> } else { >> // It's a primitive type >> return vm.primitiveTypeMirror(sig.jdwpTag());``` >> >> Thanks, > >> > Do we even need findComponentType() any more? Isn't >> > ReferenceTypeImpl.findType() sufficient. >> >> We still need findComponentType(), >> Difference between findType() and findComponentType() is that, >> findComponentType() tries to get the list of ReferenceType from the >> "vm.classesByName". In case list is empty, it explicitly throws >> ClassNotLoadedException. >> This exception check is required in validateAssignment(ValueContainer >> destination) call from ObjectReferenceImpl.java. > > I'm not sure what you mean by this. After your changes, this is all > `findComponentType()` does: > > > Type findComponentType(String signature) throws ClassNotLoadedException { > return findType(signature); > } > > > And `findType()` has the exact same signature, including the `throws`: > > ` Type findType(String signature) throws ClassNotLoadedException {` > > > So my suggestion is to get rid of `findComponentType()` and just have current > users call `findType()` instead. Thanks Chris, for the review comments. I have modified the suggested changes, kindly review. Thanks, ------------- PR: https://git.openjdk.java.net/jdk/pull/3658
