On Mon, 21 Oct 2024 21:56:11 GMT, Ioi Lam <ik...@openjdk.org> wrote: >>> I read through it fairly thoroughly. I made a lot of suggestions. Rather >>> than edit comments into lots of spots in this PR, I made the following >>> patch file. >>> [aot-indy-21143-commentary.diff.txt](https://github.com/user-attachments/files/17408940/aot-indy-21143-commentary.diff.txt) >> >> >> +bool InstanceKlass::is_enum_subclass(bool direct_only) const { >> + InstanceKlass* s = java_super(); >> + return (s == vmClasses::Enum_klass() || >> + (s != null && s->java_super() == vmClasses::Enum_klass())); >> +} >> >> Just happened to notice you aren't checking `direct_only`. > >> ``` >> +bool InstanceKlass::is_enum_subclass(bool direct_only) const { >> + InstanceKlass* s = java_super(); >> + return (s == vmClasses::Enum_klass() || >> + (s != null && s->java_super() == vmClasses::Enum_klass())); >> +} >> ``` >> >> Just happened to notice you aren't checking `direct_only`. > > I consulted with John and removed the `direct_only` parameter as it's no > longer needed.
> @iklam Changes to move main module name from modules.cpp to elsewhere do not > look related to AOT-linking of indy. If so, can they be done in a separate PR. The change is actually necessary -- the main module name used to be restored inside `MetaspaceShared::serialize()`, which happens after we decide to accept the CDS archive. When the main module doesn't match, we will disable the "archived full module graph" and keep on using the rest of the CDS archive. With aot-linked classes, the main module is needed early to decide whether to reject a CDS archive altogether. Therefore, I needed to lift its processing our of `MetaspaceShared::serialize()` and put its information directly inside the archive file header. (This change could have been included in https://github.com/openjdk/jdk/pull/20843 , but that might be too much work now that the review for JEP 483 is winding down). ------------- PR Comment: https://git.openjdk.org/jdk/pull/21143#issuecomment-2427822847
