[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8241371: Refactor and consolidate package_from_name
From:       Ioi Lam <ioi.lam () oracle ! com>
Date:       2020-03-23 19:41:49
Message-ID: b5058bbf-4a40-2288-cbcc-fa4f5818038f () oracle ! com
[Download RAW message or body]

Looks good to me. thanks!

- Ioi

On 3/23/20 12:24 PM, Claes Redestad wrote:
> Hi,
>
> fixed and cleaned up:
>
> http://cr.openjdk.java.net/~redestad/8241371/open.02/
>
> I mentioned the unnecessary name == NULL check elsewhere in this review
> thread and suggested we fix that in a follow-up.
>
> /Claes
>
> On 2020-03-23 19:31, Ioi Lam wrote:
>> Hi Claes,
>>
>> Thanks for moving the code back to classLoader.cpp. Now I can compare 
>> the difference more easily and found some issues:
>>
>>    if (*start == JVM_SIGNATURE_ARRAY) {
>>      do {
>>        start++;
>>      } while (*start == JVM_SIGNATURE_ARRAY);
>>
>> The utf8 in the symbol is not 0-terminated, so it's better to change 
>> it to
>>
>>      do {
>>        start++;
>>      } while (start < end && *start == JVM_SIGNATURE_ARRAY);
>>
>> ========
>>
>>      if (start != base && *start == JVM_SIGNATURE_CLASS) {
>>
>> We know that start will be at least (base+1), so the (start != base) 
>> is not necessary.
>>
>> =========
>>
>> Also, do we ever call ClassLoader::package_from_class_name() with a 
>> NULL name? If not, I think this can be removed
>>
>>    if (name == NULL) {
>>      if (bad_class_name != NULL) {
>>        *bad_class_name = true;
>>      }
>>      return NULL;
>>    }
>>
>> =========
>>
>>      // A package or class name could have just the slash character 
>> in the name.
>>
>> This comment is ambiguous about whether "could have" is legal or not. 
>> I think it's better to change the comment to.
>>
>>      // It's illegal for a package or class name to have just the 
>> slash character in the name.
>>
>>
>> Thanks
>> - Ioi
>>
>> On 3/23/20 3:44 AM, Claes Redestad wrote:
>>> On 2020-03-23 04:36, Ioi Lam wrote:
>>>>>>
>>>>>
>>>>> Ok, I just felt that as a static utility it's more coupled with a 
>>>>> class/InstanceKlass than its class loader. If you insist I'll move it
>>>>> to ClassLoader instead.
>>>>>
>>>> classLoader.cpp also has other methods related to packages, so I 
>>>> think it's a good place for this method, too. Plus, you don't need 
>>>> to move the test cases, so there's less code churn.
>>>
>>> Ok:
>>>
>>> http://cr.openjdk.java.net/~redestad/8241371/open.01/
>>>
>>> /Claes
>>

[prev in list] [next in list] [prev in thread] [next in thread]