Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Fri, 22 Mar 2024 12:26:25 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Revert Formatting in awt_Component.cpp >> - Revert Formatting in awt_Window.cpp > > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 779: > >> 777: } >> 778: >> 779: > > Suggestion: To be clear: I recommend deleting this line, since it does not make sense to have two consecutive blank lines here. > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1316: > >> 1314: env->CallVoidMethod(newPaper, setImageableID, ix, iy, iw, ih); >> 1315: >> 1316: > > Suggestion: I recommend deleting this line, since it does not make sense to have two consecutive blank lines here. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547831604 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547832678
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 08:44:31 GMT, Julian Waters wrote: > Maybe as a further improvement, I can inline > THROW_NULL_PDATA_IF_NOT_DESTROYED at its callsites and replace the bad > NullPointerException error message with the proper null pointer name. Since > Phil isn't here, what do you think? That might be a good future code cleanup, but I suggest you refrain from doing that in this change. > Regardless, I really hope I can get this in by Thursday. I see that you are struggling a bit to get this done. I can probably help you fix the remaining issues, but I can't push to your PR. So either I'd need to create a fork of your `patch-10` branch in my personal fork and you'll have to pull from there into your branch, or you need to give me write permissions to your personal fork. I believe you can assign permissions on a per-branch basis. I'd really like to see this PR to come to conclusion. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2021391154
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 07:44:22 GMT, Magnus Ihse Bursie wrote:
> > I have a concern since the null check bailout involves
> > THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove
> > the pData local.
>
> The name of the macro is not great, but it does not involve pData (the bad
> NPE error message notwithstanding):
>
> ```
> #define THROW_NULL_PDATA_IF_NOT_DESTROYED(peer) { \
> jboolean destroyed = JNI_GET_DESTROYED(peer); \
> if (destroyed != JNI_TRUE) { \
> env->ExceptionClear();\
> JNU_ThrowNullPointerException(env, "null pData"); \
> } \
> }
> ```
>
> So you can go ahead and replace the pData references with the variable that
> will eventually be used.
Alright, will do. Maybe as a further improvement, I can inline
THROW_NULL_PDATA_IF_NOT_DESTROYED at its callsites and replace the bad
NullPointerException error message with the proper null pointer name. Since
Phil isn't here, what do you think?
Regardless, I really hope I can get this in by Thursday. University for me
officially ramps up into _very_ high gear about that time, and I doubt I can
juggle both JDK work and it all at once by then
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019839348
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 00:13:09 GMT, Julian Waters wrote:
> I have a concern since the null check bailout involves
> THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove
> the pData local.
The name of the macro is not great, but it does not involve pData (the bad NPE
error message notwithstanding):
#define THROW_NULL_PDATA_IF_NOT_DESTROYED(peer) { \
jboolean destroyed = JNI_GET_DESTROYED(peer); \
if (destroyed != JNI_TRUE) { \
env->ExceptionClear();\
JNU_ThrowNullPointerException(env, "null pData"); \
} \
}
So you can go ahead and replace the pData references with the variable that
will eventually be used.
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019589225
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Mon, 25 Mar 2024 09:02:22 GMT, Magnus Ihse Bursie wrote:
> > The only thing I'm uncertain about is the pData local, which I don't see
> > much benefit in removing since the null check associated with it still has
> > to remain for code semantics to be correct
>
> The point is that you can do the null check on the correct variable directly,
> instead of going a detour with pData. So instead of:
>
> ```
> RealType realVal;
> void* pData = getVal()
> if (pData == null) {
> // bail out
> }
> realVal = (RealType) pData;
> ```
>
> you can just do:
>
> ```
> RealType realVal = getVal();
> if (realVal == null) {
> // bail out
> }
> ```
>
> as the code normally would have been written, had there not been an old macro
> that used the "magic" temporary pData variable.
>
> This is a recurring pattern in this patch and needs to be fixed everywhere.
I have a concern since the null check bailout involves
THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove the
pData local. Maybe Phil can suggest an alternative to that, because I don't
know what that might be. Although, if given the choice I'd use RAII through
unique_ptr like Thomas suggested, it just seems so much cleaner to me. Speaking
of, hopefully this causes enough noise to get Phil's attention again. In the
meantime, I've reverted as much of the formatting changes I could possibly find
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019140763
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Mon, 25 Mar 2024 08:59:08 GMT, Magnus Ihse Bursie wrote: >> I don't think I can commit this as there are 3 backticks at the end there :P > > Apologies. The point was that this was formatting changes that were not > needed and should be reverted. I know, I did want to commit your change directly though, just can't because of the backticks :P - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538377635
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Mon, 25 Mar 2024 05:56:52 GMT, Julian Waters wrote:
> The only thing I'm uncertain about is the pData local, which I don't see much
> benefit in removing since the null check associated with it still has to
> remain for code semantics to be correct
The point is that you can do the null check on the correct variable directly,
instead of going a detour with pData. So instead of:
RealType realVal;
void* pData = getVal()
if (pData == null) {
// bail out
}
realVal = (RealType) pData;
you can just do:
RealType realVal = getVal();
if (realVal == null) {
// bail out
}
as the code normally would have been written, had there not been an old macro
that used the "magic" temporary pData variable.
This is a recurring pattern in this patch and needs to be fixed everywhere.
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2017510861
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Mon, 25 Mar 2024 05:58:41 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1230: >> >>> 1228: jdouble imgY = (jdouble) ((yPixelOrg * 72)/(jdouble) yPixelRes); >>> 1229: jdouble imgWid = (jdouble) ((imgPixelWid * 72)/(jdouble) >>> xPixelRes); >>> 1230: jdouble imgHgt = (jdouble) ((imgPixelHgt * 72)/(jdouble) >>> yPixelRes); >> >> Suggestion: >> >> jdouble imgX = (jdouble)((xPixelOrg * 72)/(jdouble)xPixelRes); >> jdouble imgY = (jdouble)((yPixelOrg * 72)/(jdouble)yPixelRes); >> jdouble imgWid = (jdouble)((imgPixelWid * 72)/(jdouble)xPixelRes); >> jdouble imgHgt = (jdouble)((imgPixelHgt * 72)/(jdouble)yPixelRes);``` > > I don't think I can commit this as there are 3 backticks at the end there :P Apologies. The point was that this was formatting changes that were not needed and should be reverted. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1537238932
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Fri, 22 Mar 2024 12:27:31 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Revert Formatting in awt_Component.cpp >> - Revert Formatting in awt_Window.cpp > > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1230: > >> 1228: jdouble imgY = (jdouble) ((yPixelOrg * 72)/(jdouble) yPixelRes); >> 1229: jdouble imgWid = (jdouble) ((imgPixelWid * 72)/(jdouble) >> xPixelRes); >> 1230: jdouble imgHgt = (jdouble) ((imgPixelHgt * 72)/(jdouble) >> yPixelRes); > > Suggestion: > > jdouble imgX = (jdouble)((xPixelOrg * 72)/(jdouble)xPixelRes); > jdouble imgY = (jdouble)((yPixelOrg * 72)/(jdouble)yPixelRes); > jdouble imgWid = (jdouble)((imgPixelWid * 72)/(jdouble)xPixelRes); > jdouble imgHgt = (jdouble)((imgPixelHgt * 72)/(jdouble)yPixelRes);``` I don't think I can commit this as there are 3 backticks at the end there :P - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1537089274
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Wed, 20 Mar 2024 06:38:59 GMT, Julian Waters 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 incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Revert Formatting in awt_Window.cpp Thanks for the clarifications! Makes my job a lot easier. The only thing I'm uncertain about is the pData local, which I don't see much benefit in removing since the null check associated with it still has to remain for code semantics to be correct - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2017282826
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Wed, 20 Mar 2024 06:38:59 GMT, Julian Waters 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 incrementally with two additional
> commits since the last revision:
>
> - Revert Formatting in awt_Component.cpp
> - Revert Formatting in awt_Window.cpp
src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp line 44:
> 42: #include
> 43:
> 44: // Don't want to pull in the redefined allocation functions
Maybe this needs to be clarified a bit:
Suggestion:
// These files must be included before awt.h, since the latter redefines malloc
// to Do_Not_Use_Malloc, etc, and that will break these files.
Also, please mark conversations as resolved in the GitHub UI once you think
they are addressed. That makes it much easier for reviewers to understand what
parts of the reviews that have not been addressed.
src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp line 31:
> 29:
> 30: // Don't want to pull in the redefined allocation functions
> 31: #include "awt_ole.h"
Suggestion:
// awt_ole.h must be included before awt.h, since the latter redefines malloc
// to Do_Not_Use_Malloc, etc, and that will break awt_ole.h.
#include "awt_ole.h"
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 544:
> 542: and for all other dialogs AwtToolkit's HWND is used.
> 543: */
> 544: else if (awtParent != NULL) {
Suggestion:
else if (awtParent != NULL)
{
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 546:
> 544: else if (awtParent != NULL) {
> 545: setup.hwndOwner = AwtToolkit::GetInstance().GetHWnd();
> 546: } else {
Suggestion:
}
else
{
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 779:
> 777: }
> 778:
> 779:
Suggestion:
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1230:
> 1228: jdouble imgY = (jdouble) ((yPixelOrg * 72)/(jdouble) yPixelRes);
> 1229: jdouble imgWid = (jdouble) ((imgPixelWid * 72)/(jdouble) xPixelRes);
> 1230: jdouble imgHgt = (jdouble) ((imgPixelHgt * 72)/(jdouble) yPixelRes);
Suggestion:
jdouble imgX = (jdouble)((xPixelOrg * 72)/(jdouble)xPixelRes);
jdouble imgY = (jdouble)((yPixelOrg * 72)/(jdouble)yPixelRes);
jdouble imgWid = (jdouble)((imgPixelWid * 72)/(jdouble)xPixelRes);
jdouble imgHgt = (jdouble)((imgPixelHgt * 72)/(jdouble)yPixelRes);```
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1316:
> 1314: env->CallVoidMethod(newPaper, setImageableID, ix, iy, iw, ih);
> 1315:
> 1316:
Suggestion:
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3153:
> 3151: }
> 3152:
> 3153: window = (AwtWindow *) pData;
Suggestion:
window = (AwtWindow *)pData;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535490288
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535491034
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493157
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493362
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493818
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535494973
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535495291
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535496714
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
> 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 incrementally with two additional commits since the last revision: - Revert Formatting in awt_Component.cpp - Revert Formatting in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/18014c3a..0439b138 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=51 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=50-51 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
