RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
Phil Race
prr at openjdk.org
Sat Jan 20 00:44:07 UTC 2024
On Thu, 11 Jan 2024 08:24:42 GMT, Julian Waters <jwaters at 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
More information about the client-libs-dev
mailing list