RFR: 8241371: Refactor and consolidate package_from_name

[Ioi Lam]

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