8366897: RBTreeTest.IntrusiveCustomVerifyTest and RBTreeTest.CustomVerify tests fail on non-debug builds

[caspernorrbin]

#26981 introduced the possibility to supply the red-black tree with a custom verifier, however the call to this verifier was inside an assert, meaning it wasn't called for all builds. This results in two gtests failing.

This fix moves the call outside the assert, and then checks the result instead.

Testing:

• Local gtest testing
• Oracle tier 1

---

Progress

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

Issue

• JDK-8366897: RBTreeTest.IntrusiveCustomVerifyTest and RBTreeTest.CustomVerify tests fail on non-debug builds (Bug - P4)

Reviewers

• Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27097

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27097.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@caspernorrbin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8366897: RBTreeTest.IntrusiveCustomVerifyTest and RBTreeTest.CustomVerify tests fail on non-debug builds

Reviewed-by: ayang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

• 1dc1d56: 8363858: [perf] OptimizeFill may use wide set of intrinsics
• 8520fd3: 8366365: [test] test/lib-test/jdk/test/whitebox/CPUInfoTest.java should be updated
• f90d520: 8366475: Rename MetaspaceShared class to AOTMetaspace
• ... and 3 more: https://git.openjdk.org/jdk/compare/8c50bed86709a45615743dd7953b8c6861f1da0c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@caspernorrbin The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (3b370287)

[albertnetymk]

Since extra_verifier may have side-effect, it must be invoked outside the assert macro.

How about assert(verifier(... a few below? Does that require similar fixup?

[caspernorrbin]

How about assert(verifier(... a few below? Does that require similar fixup?

That is the internal self-defined verifier that compares nodes, defined in verify_self. It has no side effect, and is strictly used for node comparisons.

[albertnetymk]

It has no side effect, and is strictly used for node comparisons.

I see; seems a bit fragile though -- it's not obvious that this closure must be side-effect free.

Anyway, this fix is trivial enough; feel free to merge to reduce CI noise.

[caspernorrbin]

I see; seems a bit fragile though -- it's not obvious that this closure must be side-effect free.

I can understand that. But seeing as it's a one-line lambda in the function calling verify, I think it's mostly fine. Thanks for reviewing.

/integrate

[openjdk]

Going to push as commit 945aaf8.
Since your change was applied there have been 6 commits pushed to the master branch:

• 1dc1d56: 8363858: [perf] OptimizeFill may use wide set of intrinsics
• 8520fd3: 8366365: [test] test/lib-test/jdk/test/whitebox/CPUInfoTest.java should be updated
• f90d520: 8366475: Rename MetaspaceShared class to AOTMetaspace
• ... and 3 more: https://git.openjdk.org/jdk/compare/8c50bed86709a45615743dd7953b8c6861f1da0c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@caspernorrbin Pushed as commit 945aaf8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.