RFR: 8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means [v6]

Ioi Lam iklam at openjdk.java.net
Mon May 17 06:39:39 UTC 2021


On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.
>> 
>> Tests:
>> - [x] tier1, 2
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   declare has_old_class_version as a const method

> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):_
> 
> On 15/05/2021 7:14 am, Ioi Lam wrote:
> 
> > On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:
> > > > Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.
> > > > Tests:
> > > > - [x] tier1, 2
> > > 
> > > 
> > > Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> > > declare has_old_class_version as a const method
> > 
> > 
> > > _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):_
> > > Hi Calvin,
> > > On 12/05/2021 2:23 am, Calvin Cheung wrote:
> > > > Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.
> > > 
> > > 
> > > I guess you missed the fact that I had changed the bug synopsis. Why not
> > > call this needs_old_verifier (or something like that) so that the exact
> > > meaning of this method is more clear?
> > 
> > 
> > needs_old_verifier isn't clear, either. `has_old_class_version(K)` returns true if K is "new" but one of K's supertypes is "old". K itself does not "need the old verifier".
> 
> and k itself does not "have an old class version" so the current method
> is even more misleadingly named!
> 
> How about inverting it and having has_post_Java_6_hierarchy: return true
> if k and all its supertypes are post-Java-6 classfile version.

It's unclear whether hierarchy refers to the super types, subtypes, or both.

> Seriously we must be able to name this method in a more meaningful way. :(

Trying to describe what the function does in a short and meaningful name is not always possible.

In this case, I think it's better to say what the function intends to do. `InstanceKlass::can_be_verified_at_dumptime()` will probably suffice. It's unlikely that this function will be used for any other purpose.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3983


More information about the hotspot-runtime-dev mailing list