On Thu, 11 Jan 2024 08:24:42 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 79 commits: > > - Merge branch 'openjdk:master' into patch-10 > - Merge branch 'openjdk:master' into patch-10 > - Fix awt_Window.cpp > - Fix awt_PrintJob.cpp > - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4 > - Fix awt_Window.cpp > - awt_Window.cpp > - awt_PrintJob.cpp > - awt_Frame.cpp > - Whitespace awt_Component.cpp > - ... and 69 more: https://git.openjdk.org/jdk/compare/35e96627...cbfbaee6 Changes requested by prr (Reviewer). src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 206: > 204: > 205: void AwtCanvas::_SetEraseBackground(void *param) { > 206: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); Nits (1) I see no reason to have moved the { - it is now inconsistent with all the rest of the code. (2) Why do we need a SPACE for the cast ? It isn't usual. These things just add to the changes that need to be looked at. Please revert. src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6361: > 6359: void AwtComponent::_SetParent(void * param) { > 6360: if (AwtToolkit::IsMainThread()) { > 6361: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); superflous space again src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6366: > 6364: jobject parent = data->parentComp; > 6365: > 6366: AwtComponent *awtComponent = nullptr; Looking at it (not tested) here through 6403 could be simplified as if (self == NULL || parent == NULL) { env->ExceptionClear(); JNU_ThrowNullPointerException(env, "peer"); env->DeleteGlobalRef(self); env->DeleteGlobalRef(parent); delete data; return; } AwtComponent *awtComponent = (AwtComponent *)JNI_GET_PDATA(self); if (awtComponent == NULL) { THROW_NULL_PDATA_IF_NOT_DESTROYED(self); env->DeleteGlobalRef(self); env->DeleteGlobalRef(parent); delete data; return; } AwtComponent *awtParent = (AwtComponent *)JNI_GET_PDATA(parent); if (awtParent == NULL) { THROW_NULL_PDATA_IF_NOT_DESTROYED(parent); env->DeleteGlobalRef(self); env->DeleteGlobalRef(parent); delete data; return; } I think I prefer that over BOOL error = FALSE; AwtComponent *awtComponent = nullptr; AwtComponent *awtParent = nullptr; if (self == NULL || parent == NULL) { env->ExceptionClear(); JNU_ThrowNullPointerException(env, "peer"); error = TRUE; } if (!error) { awtComponent = (AwtComponent *)JNI_GET_PDATA(self); if (awtComponent == NULL) { THROW_NULL_PDATA_IF_NOT_DESTROYED(self); error = TRUE; } } if (!error) { awtParent = (AwtComponent *)JNI_GET_PDATA(parent); if (awtParent == NULL) { THROW_NULL_PDATA_IF_NOT_DESTROYED(parent); error = TRUE; } if (error) { env->DeleteGlobalRef(self); env->DeleteGlobalRef(parent); delete data; return; } src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1340: > 1338: > 1339: void AwtFrame::_SetState(void *param) { > 1340: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); more un-needed reformatting src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1365: > 1363: f = (AwtFrame *) pData; > 1364: HWND hwnd = f->GetHWnd(); > 1365: if (::IsWindow(hwnd)) { more formatting to revert src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1581: > 1579: void AwtFrame::_NotifyModalBlocked(void *param) > 1580: { > 1581: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); again .. src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1600: > 1598: return; > 1599: } else { > 1600: pData = JNI_GET_PDATA(peer); like in my recoded case above, we no longer need the "pData" var which was there only because that name is magically known to the macro. src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1624: > 1622: return; > 1623: } else { > 1624: pData = JNI_GET_PDATA(blockerPeer); can directly set dialog src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1018: > 1016: > 1017: void AwtWindow::_RepositionSecurityWarning(void* param) { > 1018: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); formatting .. src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1032: > 1030: } else { > 1031: pData = JNI_GET_PDATA(self); > 1032: if (pData == NULL) { directly set window src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3127: > 3125: > 3126: void AwtWindow::_ModalDisable(void *param) { > 3127: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); formatting src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3137: > 3135: > 3136: PDATA pData = JNI_GET_PDATA(self); > 3137: if (self == NULL) { Surely line 3136 above should have been deleted ? It is replaced by line 3143 below. And then you can also directly set window, pData isn't needed, as discussed in similar cases above src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3163: > 3161: > 3162: void AwtWindow::_ModalEnable(void *param) { > 3163: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); fomatting src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3171: > 3169: > 3170: PDATA pData = JNI_GET_PDATA(self); > 3171: if (self == NULL) { like the method above line 3170 seems wrong. src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3223: > 3221: > 3222: void AwtWindow::_SetOpaque(void* param) { > 3223: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); formatting src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3237: > 3235: } else { > 3236: pData = JNI_GET_PDATA(self); > 3237: if (pData == NULL) { set directly src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3253: > 3251: > 3252: void AwtWindow::_UpdateWindow(void* param) { > 3253: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2); formatting src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3269: > 3267: return; > 3268: } else { > 3269: pData = JNI_GET_PDATA(self); set directly src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3308: > 3306: return; > 3307: } else { > 3308: pData = JNI_GET_PDATA(self); set directly ------------- PR Review: https://git.openjdk.org/jdk/pull/15096#pullrequestreview-1833960495 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460013033 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460023492 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460050708 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460082135 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460082645 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460082873 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460083533 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460084160 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460085276 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460085458 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460085719 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460087127 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460087785 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460088414 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460088776 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460088996 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460089073 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460089276 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1460089490
