On Thu, 11 Dec 2025 11:51:00 GMT, Axel Boldt-Christmas <[email protected]> wrote:
> This is a enhancement which should also go into mainline, however since > lworld is changing a lot of the Klass type hierarchy it seems more important > to just get this into this repo. > > Using override provides better visibility and compiler support for suspicious > virtual methods. It makes it clears when reading a class header file if a new > virtual interface is introduced or if it simply overridden. > > It catches when the virtual hierarchy is different for different build > configurations (like `is_flatArray_klass_slow` which is on the `Klass` in > debug and on `FlatArrayKlass` in release. This is obviously a bug and > `is_flatArray_klass_slow` should not be available in a release build) or > suspicious virtual functions (like > `InstanceKlass::restore_unshareable_info(ClassLoaderData*,Handle,PackageEntry*, > JavaThread*)` which is only overridden by `InlineKlass` and only delegates > to `InstanceKlass`, so it could just not have overridden this function). > > We should aim to migrate all our C++ classes to use `override` or `final` > rather than virtual to catch more bugs and have more expressive header files. > But it seems extra prudent here as lworld is moving around a lot of things in > this area. I agree that `override` (sometimes `final`) is strictly better than virtual when it applies, and that's a good direction to take. I find unfortunate not to do that in mainline as well, the PR description explains why it's more important here, but I fear it creates opportunities for conflicts during our jdk merges. Another possible issue is when a virtual method is added on mainline, with an override that is not marked with `override`, after the jdk merge, it will fail compilation on Mac (only?) because there is a warning about inconsistent use of override on this platform (if a class has one override, all the methods that can have it must have it). While I would agree the former seems rather unlikely and easy to fix, the latter seems less crazy and more annoying as it will not show up at merge-time, or local build on Linux, for instance. Still, not hard to fix. While problems I imagine are rather easy to solve, they might come more than once, and be an annoyance on the way of merging (that seems already painful enough as it is). I can imagine that the urgency of getting this change in valhalla justifies not to put it in mainline and wait for the next jdk merge, but is it planned to do the same in mainline soon, to avoid mentioned issues, and so mainline can also benefit from the change? If so, it would be nice to have a JBS issue to track that. ------------- PR Comment: https://git.openjdk.org/valhalla/pull/1787#issuecomment-3645741413
