RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()

[David Holmes]

Hi Calvin,

On 5/10/2017 3:12 AM, Calvin Cheung wrote:
> Mandy, David,
> 
> Thanks for the review.
> 
> I've made the change based on your suggestion:
> http://cr.openjdk.java.net/~ccheung/8185694/webrev.01/

share/classfile/systemDictionary.cpp

There's an existing bug that this local is not used:

132   Klass* system_klass = WK_KLASS(ClassLoader_klass);

(and the variable is poorly named). Now that you need to use 
WK_KLASS(ClassLoader_klass) twice you should instead use the local.

---

As Mandy alludes these:

177 // Returns true if the passed class loader is the builtin 
application class loader
  178 // or a custom system class loader. A customer system class loader 
can be
  179 // specified via -Djava.system.class.loader.
  180 bool SystemDictionary::is_system_class_loader(oop class_loader) {
  181   if (class_loader == NULL) {
  182     return false;
  183   }
  184   return (class_loader->klass() == 
SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() ||
  185           class_loader == _java_system_loader);
  186 }
  187
  188 // Returns true if the passed class loader is the platform class 
loader.
  189 bool SystemDictionary::is_platform_class_loader(oop class_loader) {
  190   if (class_loader == NULL) {
  191     return false;
  192   }
  193   return (class_loader->klass() == 
SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass());
  194 }

would seem to be checking the wrong things - they should just be 
checking java_system_loader() and java_platform_loader() shouldn't they?

In the case of is_system_class_loader() it seems odd that it checks a 
specific hard-wired class as well as the defined _java_system_loader ??

Thanks,
David
-----



> thanks,
> Calvin
> 
> On 10/4/17, 8:29 AM, mandy chung wrote:
>>
>>
>> On 10/3/17 11:36 PM, David Holmes wrote:
>>> Hi Calvin,
>>>
>>> On 4/10/2017 7:26 AM, Calvin Cheung wrote:
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8185694
>>>>
>>>> The open code change for this fix involves adding the 
>>>> _java_platform_loader to the SystemDictionary class.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8185694/webrev.00/
>>>
>>> I think you should be calling getPlatformClassLoader rather than 
>>> poking at the parent field of the system loader. That way you don't 
>>> have to worry about whether or not the system loader has been 
>>> customized by the user.
>> Agree.  ClassLoader::getPlatformClassLoader is the right way to get 
>> the platform class loader.
>>
>> Mandy