8373507: [lworld] Add override keyword to *Klass classes

[xmas92]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8373507: [lworld] Add override keyword to *Klass classes (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1787/head:pull/1787
$ git checkout pull/1787

Update a local copy of the PR:
$ git checkout pull/1787
$ git pull https://git.openjdk.org/valhalla.git pull/1787/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1787

View PR using the GUI difftool:
$ git pr show -t 1787

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1787.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back aboldtch! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 00: Full (accb06d6)

[marc-chevalier]

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.