On Mon, 24 Jun 2024 20:16:30 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
> This is somewhat a continuation for > [JDK-8307160](https://bugs.openjdk.org/browse/JDK-8307160) and > [JDK-8334509](https://bugs.openjdk.org/browse/JDK-8334509). > > The former removed the `doIt` flag in #18584, but it introduced a regression. > > The regression is resolved by the latter in #19786, and it added the 'doIt' > flag again. > > I think using a flag makes the code less clear. When reading the code, one > has to keep track of the current value of the `doIt` flag. > > I raised my concern in [comments in PR > 19786](https://github.com/openjdk/jdk/pull/19786#discussion_r1649191308): > >> I'd rather keep `JNI_FALSE` here — it's more explicit and therefore clearer. >> You don't have to keep track of the value of `doIt` flag while reading the >> code. >> >> In fact, I'd prefer no `doIt` flag at all… yet it makes handling the code >> below `if (ret)` slightly harder. > > I came up with a solution which simplifies handling the clean-up. The > solution relies on C++ destructors which are called when the declared objects > go out of scope. > > The C++ object wraps a lambda function to clean up and invokes this function > in its destructor. Such C++ class has to be a templated class because there's > no defined type to represent a lambda. The class has to be declared at the > top of the file because it needs C++ linkage. > > There are two `Cleaner` objects declared in the code of > `Java_sun_awt_windows_WPageDialogPeer__1show`: > > **`refCleaner`** is declared right after all the references to Java objects > are initialised. The corresponding `refCleanup` lambda deletes all the > references and replaces the `CLEANUP_SHOW` macro. Before JDK-8307160, the > code jumped to a go-to label to perform the clean-up. > > **`postCleaner`** is declared after `AwtDialog::CheckInstallModalHook` is > called. Its lambda `postCleanup` uninstalls the modal hook and handles focus > transfer as well as saves the updated printer parameters. > > It is `postCleaner` that resolves the bug JDK-8334868 by ensuring > `CheckUninstallModalHook` and `ModalActivateNextWindow` are called if > `::PageSetupDlg` is called. > > As the result of using `refCleaner`, all the `return` statements in the > function use an explicit value, which makes the code cleaner. There's no need > to use a `go to` statement or insert a macro to delete references to Java > objects, about which one had to remember — the destructor of the `refCleaner` > handles deleting references when `refCleaner` goes out of scope, that is when > the function returns. Sorry, but I don't see this as better or needed. I was the one who requested the current code and I don't see the need for all this complexity. Just do the minimum change to make sure we do CheckUninstallModalHook is called. ------------- Changes requested by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19867#pullrequestreview-2169910991
