RFR: 8241371: Refactor and consolidate package_from_name

[Claes Redestad]

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
>