Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Wed, 3 Apr 2024 02:38:21 GMT, Julian Waters wrote: >> Bumping > >> @TheShermanTanker I tried to help you get this done. I added fixes to a copy >> of your branch on my personal fork, but then it turned out I could not push >> them to your branch. :-( >> >> It ended up with me creating a new PR, #18584. As a bonus, I think it might >> be easier to review with a fresh start. This PR has grown quite heavy with >> lots of comments and commits. >> >> I hope you don't feel like I'm stealing this away from you. You have done a >> great job, and shown a lot of patience of carrying this all the way here. >> But I also got the impression that you would appreciate my assistance in >> getting the last pieces in place so we can integrate this. > > Not at all, I don't feel like you're stealing this from me. In fact, I should > be the one apologising for giving you extra work! Thanks for taking this up, > I once again apologise for making you do this instead, I've been very busy > since Thursday (working on OpenJDK while in lectures at times), and during my > breaks I've been too drained to continue, so i really appreciate your help :) > > I will keep this open until the other Pull Request has been integrated, in > case this might still be needed @TheShermanTanker You can close this PR. The bug was fixed with https://github.com/openjdk/jdk/pull/18584 instead. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2090226777
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
On Fri, 5 Apr 2024 05:48:40 GMT, Julian Waters wrote:
>> Magnus Ihse Bursie has updated the pull request with a new target base due
>> to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains two
>> additional commits since the last revision:
>>
>> - Merge branch 'master' into awt-permissive-minus
>> - 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft
>> Visual C compiler
>
> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6370:
>
>> 6368: AwtComponent *awtParent = NULL;
>> 6369:
>> 6370: if (self == NULL) {
>
> I had missed this in my earlier sweep of the changes, but didn't Phil request
> that both the check for self and parent be merged into self == NULL || parent
> == NULL? (Or as I'd prefer, nullptr)
I noticed Phil's comment, but I could not make it look nice. For instance, the
NPE error message was unclear, and you could not see the connection between
self->awtComponent and parent->awtParent, which was obvious in the original
code.
In the end, I thought it was preferable to use the exact same style when
expanding the macro as elsewhere. That creates a common pattern across the
codebase.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18584#discussion_r1553174771
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
On Thu, 4 Apr 2024 22:47:18 GMT, Magnus Ihse Bursie wrote:
>> This is a retake on https://github.com/openjdk/jdk/pull/15096.
>>
>> I tried to fix the remaining issues in that PR, but could not push them. In
>> the end, it seemed easier to create a new branch in my own personal fork.
>>
>> The majority of the work in this PR has been done by @TheShermanTanker. I
>> have stepped in and fixed the remaining comments, reverted some additional
>> unneeded changed code, and also tried to minimize code duplication in
>> `awt_PrintJob.cpp`.
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains two additional
> commits since the last revision:
>
> - Merge branch 'master' into awt-permissive-minus
> - 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft
> Visual C compiler
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6370:
> 6368: AwtComponent *awtParent = NULL;
> 6369:
> 6370: if (self == NULL) {
I had missed this in my earlier sweep of the changes, but didn't Phil request
that both the check for self and parent be merged into self == NULL || parent
== NULL? (Or as I'd prefer, nullptr)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18584#discussion_r1552961774
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
> This is a retake on https://github.com/openjdk/jdk/pull/15096. > > I tried to fix the remaining issues in that PR, but could not push them. In > the end, it seemed easier to create a new branch in my own personal fork. > > The majority of the work in this PR has been done by @TheShermanTanker. I > have stepped in and fixed the remaining comments, reverted some additional > unneeded changed code, and also tried to minimize code duplication in > `awt_PrintJob.cpp`. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into awt-permissive-minus - 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler - Changes: - all: https://git.openjdk.org/jdk/pull/18584/files - new: https://git.openjdk.org/jdk/pull/18584/files/e9780f0c..868dc56d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18584&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18584&range=00-01 Stats: 9831 lines in 307 files changed: 3321 ins; 4776 del; 1734 mod Patch: https://git.openjdk.org/jdk/pull/18584.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18584/head:pull/18584 PR: https://git.openjdk.org/jdk/pull/18584
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler
On Tue, 2 Apr 2024 15:45:59 GMT, Magnus Ihse Bursie wrote: > This is a retake on https://github.com/openjdk/jdk/pull/15096. > > I tried to fix the remaining issues in that PR, but could not push them. In > the end, it seemed easier to create a new branch in my own personal fork. > > The majority of the work in this PR has been done by @TheShermanTanker. I > have stepped in and fixed the remaining comments, reverted some additional > unneeded changed code, and also tried to minimize code duplication in > `awt_PrintJob.cpp`. Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18584#pullrequestreview-1981217028
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Wed, 3 Apr 2024 02:38:21 GMT, Julian Waters wrote: > I will keep this open until the other Pull Request has been integrated, in > case this might still be needed I don't think it is necessary. You can always re-open a PR if it should be needed, and you can look at source code and comments (and even make new comments) on a closed PR. So I think it will just make it easier for everybody if there is only a single open PR for this issue. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2033880103
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler
On Tue, 2 Apr 2024 15:45:59 GMT, Magnus Ihse Bursie wrote: > This is a retake on https://github.com/openjdk/jdk/pull/15096. > > I tried to fix the remaining issues in that PR, but could not push them. In > the end, it seemed easier to create a new branch in my own personal fork. > > The majority of the work in this PR has been done by @TheShermanTanker. I > have stepped in and fixed the remaining comments, reverted some additional > unneeded changed code, and also tried to minimize code duplication in > `awt_PrintJob.cpp`. Looks good. Thanks for taking this up for me! In the future I hope @prrace will change his mind and allow the use of C++ Libraries in awt (awt already depends on the C++ Standard Library on Windows, verifiable if you run dumpbin -DEPENDENTS on it), because I have a change in mind relying on RAII that would look much cleaner and neater - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18584#pullrequestreview-1975384014
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Mon, 1 Apr 2024 10:33:41 GMT, Julian Waters wrote: >> Julian Waters has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Labels to empty line in awt_Window.cpp >> - Labels to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp > > Bumping > @TheShermanTanker I tried to help you get this done. I added fixes to a copy > of your branch on my personal fork, but then it turned out I could not push > them to your branch. :-( > > It ended up with me creating a new PR, #18584. As a bonus, I think it might > be easier to review with a fresh start. This PR has grown quite heavy with > lots of comments and commits. > > I hope you don't feel like I'm stealing this away from you. You have done a > great job, and shown a lot of patience of carrying this all the way here. But > I also got the impression that you would appreciate my assistance in getting > the last pieces in place so we can integrate this. Not at all, I don't feel like you're stealing this from me. In fact, I should be the one apologising for giving you extra work! Thanks for taking this up, I once again apologise for making you do this instead, I've been very busy since Thursday (working on OpenJDK while in lectures at times), and during my breaks I've been too drained to continue, so i really appreciate your help :) I will keep this open until the other Pull Request has been integrated, in case this might still be needed - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2033427284
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Mon, 1 Apr 2024 10:33:41 GMT, Julian Waters wrote: >> Julian Waters has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Labels to empty line in awt_Window.cpp >> - Labels to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp > > Bumping @TheShermanTanker I tried to help you get this done. I added fixes to a copy of your branch on my personal fork, but then it turned out I could not push them to your branch. :-( It ended up with me creating a new PR, https://github.com/openjdk/jdk/pull/18584. As a bonus, I think it might be easier to review with a fresh start. This PR has grown quite heavy with lots of comments and commits. I hope you don't feel like I'm stealing this away from you. You have done a great job, and shown a lot of patience of carrying this all the way here. But I also got the impression that you would appreciate my assistance in getting the last pieces in place so we can integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2032430773
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 [v46]
On Sat, 20 Jan 2024 00:38:04 GMT, Phil Race wrote:
>> 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
>
> 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
Yes.
@TheShermanTanker This is in fact a serious bug. You try to do `pData =
JNI_GET_PDATA(self)` before the null check of self, and this will crash if self
is null.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547836085
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Thu, 28 Mar 2024 07:36:04 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 four additional
> commits since the last revision:
>
> - Labels to empty line in awt_Window.cpp
> - Labels to empty line in awt_Window.cpp
> - Label to empty line in awt_Window.cpp
> - Label to empty line in awt_Window.cpp
src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 234:
> 232: c->m_eraseBackgroundOnResize = doEraseOnResize;
> 233:
> 234:
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_Component.cpp line 6586:
> 6584: component->GetInsets(gis->insets);
> 6585:
> 6586:
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_Frame.cpp line 1603:
> 1601: } else {
> 1602: f = (AwtFrame *) JNI_GET_PDATA(peer);
> 1603: if (f == nullptr) {
@prrace What is the current take on `NULL` vs `nullptr` in client code? I know
Hotspot made an effort to completely purge all `NULL` references.
The new code here uses a mix of `NULL` and `nullprt`. Should it:
a) only use `NULL`
b) only use `nullptr`
c) remain as it is
?
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 214:
> 212: static const double POINTS_TO_LOMETRIC = (254.0 / 72.0);
> 213:
> 214:
I recommend deleting one more line, since otherwise you will now have three
consecutive blank lines.
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1031:
> 1029: window->RepositionSecurityWarning(env);
> 1030:
> 1031: ret:
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_Window.cpp line 3159:
> 3157: }
> 3158:
> 3159:
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_Window.cpp line 3193:
> 3191: }
> 3192:
> 3193:
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_Window.cpp line 3224:
> 3222: window->SetTranslucency(iOpacity, window->isOpaque());
> 3223:
> 3224:
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_Window.cpp line 3255:
> 3253: window->SetTranslucency(window->getOpacity(), isOpaque);
> 3254:
> 3255:
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_Window.cpp line 3293:
> 3291: uws->hBitmap);
> 3292:
> 3293:
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_Window.cpp line 3328:
> 3326: window->setFullScreenExclusiveModeState(state != 0);
> 3327:
> 3328:
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_r1547824279
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547824797
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547829415
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547830886
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547833185
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547836998
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547837189
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547837344
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547837450
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547837625
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1547837761
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Thu, 28 Mar 2024 07:36:04 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 four additional > commits since the last revision: > > - Labels to empty line in awt_Window.cpp > - Labels to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp I've been monitoring it but saw no reason to pay particular attention until everything I raised is addressed. Including the cases identical to the ones I explicitly identified (which I suppose is what is meant by your question "should I do the unmarked ones too ?"). At some point you do tire of typing "ditto" :-) It has been long enough that I will need a little time to remember and re-review properly once the requested changes are in place. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2031068525
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Thu, 28 Mar 2024 07:36:04 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 four additional > commits since the last revision: > > - Labels to empty line in awt_Window.cpp > - Labels to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp Yes, fix all pData in areas touched by your changes. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2030503535
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Thu, 28 Mar 2024 07:36:04 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 four additional > commits since the last revision: > > - Labels to empty line in awt_Window.cpp > - Labels to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp Bumping - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2029552898
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 19:50:16 GMT, Phil Race wrote: >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > > See my comment > "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." > > so skip (and get rid of) the pData var and just set the target var directly I've switched the marked cases to use direct setting and bypass pData, should I do the unmarked ones too? @prrace - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2024592358
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
> 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 four additional commits since the last revision: - Labels to empty line in awt_Window.cpp - Labels to empty line in awt_Window.cpp - Label to empty line in awt_Window.cpp - Label to empty line in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/ee0297a6..04e3cce1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=69 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=68-69 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v69]
> 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 one additional commit since the last revision: 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/45de8955..ee0297a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=68 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=67-68 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v68]
> 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: - Label to empty line in awt_Component.cpp - Label to empty line in awt_Canvas.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/6027b1df..45de8955 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=67 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=66-67 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v67]
> 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 one additional commit since the last revision: Circumvent pData in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/360894b5..6027b1df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=66 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=65-66 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v66]
> 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 one additional commit since the last revision: Circumvent pData in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/c4f4e54b..360894b5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=65 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=64-65 Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v65]
On Wed, 27 Mar 2024 07:13:11 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 one additional > commit since the last revision: > > Circumvent pData in awt_Window.cpp Let's fix all the known problems first, and he'll return. (Otherwise I'll ping him in a different channel.) - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2022278178
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v65]
> 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 one additional commit since the last revision: Circumvent pData in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/8f4d9e95..c4f4e54b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=64 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=63-64 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v64]
> 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 one additional commit since the last revision: Circumvent pData in awt_Frame.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/da0c09b1..8f4d9e95 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=63 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=62-63 Stats: 7 lines in 1 file changed: 0 ins; 3 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v63]
> 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 one additional commit since the last revision: Circumvent pData in awt_Component.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/8ddb241d..da0c09b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=62 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=61-62 Stats: 16 lines in 1 file changed: 0 ins; 11 del; 5 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Tue, 26 Mar 2024 20:03:36 GMT, Magnus Ihse Bursie wrote: >> I see the advantage of collapsing self and parent into the same check, but >> it doesn't seem like getting rid of pData is of much benefit, the checks for >> null seem to remain the same either way > >> Sorry, I don't see a BOOL error anywhere? > > I think Phil misplaced the code block marker -- the BOOL error line and below > is supposed to be inside the second code block. > > What Phil is doing is trying to suggest two different approaches to make this > code nicer, the first with a bit more duplication, and the second with an > `error` boolean flag. And he says that he prefers the former, so the second > were propably just mentioned to show an alternative for discussion. > > I think what you should do here is to combine the `self` and `parent` null > checks as Phil suggests. And then replace all pData references to the final > variable, here as well as everywhere pData is involved. Ah, I see. Alright, will do so - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1540550556
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
On Tue, 26 Mar 2024 08:55:56 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: > > - Whitespace in awt_DnDDS.cpp > - Whitespace in awt_DnDDT.cpp Thanks, will assign the permissions. Though, I'll try to finish this myself so you don't have to- It seems like bad etiquette to make you fix something that I started. But all of this will still be meaningless if Phil doesn't return to this Pull Request any time soon :( - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2022058697
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v62]
> 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 one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/ef9f3a62..8ddb241d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=61 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=60-61 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
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 [v46]
On Wed, 20 Mar 2024 06:44:02 GMT, Julian Waters wrote: >> Sorry, I don't see a BOOL error anywhere? > > I see the advantage of collapsing self and parent into the same check, but it > doesn't seem like getting rid of pData is of much benefit, the checks for > null seem to remain the same either way > Sorry, I don't see a BOOL error anywhere? I think Phil misplaced the code block marker -- the BOOL error line and below is supposed to be inside the second code block. What Phil is doing is trying to suggest two different approaches to make this code nicer, the first with a bit more duplication, and the second with an `error` boolean flag. And he says that he prefers the former, so the second were propably just mentioned to show an alternative for discussion. I think what you should do here is to combine the `self` and `parent` null checks as Phil suggests. And then replace all pData references to the final variable, here as well as everywhere pData is involved. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1540025427
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
On Tue, 26 Mar 2024 08:55:56 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:
>
> - Whitespace in awt_DnDDS.cpp
> - Whitespace in awt_DnDDT.cpp
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 598:
> 596: int sz = GetLocaleInfo(LOCALE_USER_DEFAULT, LOCALE_IMEASURE,
> NULL, 0);
> 597: if (sz > 0) {
> 598: LPTSTR str = (LPTSTR) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> sizeof(TCHAR), sz);
Suggestion:
LPTSTR str = (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
sizeof(TCHAR), sz);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538833940
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
> 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: - Whitespace in awt_DnDDS.cpp - Whitespace in awt_DnDDT.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/90d9096f..ef9f3a62 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=60 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=59-60 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v60]
> 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: - Comment in src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp Co-authored-by: Magnus Ihse Bursie - Comment in src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/1341d2e1..90d9096f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=59 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=58-59 Stats: 5 lines in 2 files changed: 3 ins; 0 del; 2 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
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 [v59]
> 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 one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/53e8bd6d..1341d2e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=58 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=57-58 Stats: 4 lines in 1 file changed: 0 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v58]
> 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 one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/0c409d45..53e8bd6d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=57 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=56-57 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
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 [v57]
On Tue, 26 Mar 2024 00:17:56 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 one additional > commit since the last revision: > > Revert formatting change in > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp > > Co-authored-by: Magnus Ihse Bursie src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 230: > 228: } > 229: > 230: AwtCanvas *c = (AwtCanvas*) pData; Suggestion: AwtCanvas *c = (AwtCanvas*)pData; - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538714268
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v57]
> 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 one additional commit since the last revision: Revert formatting change in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/bf12c71a..0c409d45 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=56 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=55-56 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 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
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 [v56]
> 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: - Partially revert formatting in AccessBridgeJavaEntryPoints.cpp - Partially revert formatting in AccessBridgeJavaEntryPoints.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/dfc3c491..bf12c71a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=55 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=54-55 Stats: 4 lines in 1 file changed: 0 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v55]
> 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 one additional commit since the last revision: Revert formatting in awt_Frame.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/1dbd5f7e..dfc3c491 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=54 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=53-54 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v54]
> 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 src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/663a7f34..1dbd5f7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=53 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=52-53 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 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
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 [v53]
> 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 one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_Window.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/0439b138..663a7f34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=52 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=51-52 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Fri, 22 Mar 2024 10:49:13 GMT, Julian Waters wrote: >> bot-keep-alive >> >> @TheShermanTanker Did you understand the remaining changes that Phil has >> requested? Do you think you'll be able to fix this? > > @magicus Phil doesn't seem to be responding to my queries, I'm not too sure > what to do at this point @TheShermanTanker Phil is a busy man. I've helped you out (hopefully!) by being more concrete about what you need to fix, and reminded you on fixes that you seemed to have missed. Also, please use the functionality to resolve conversations, it really helps to see what remains to be fixed. (In OpenJDK, the Github project is configured so only the owner of a PR can resolve conversations.) Also, you can double check the PR yourself to see if you have introduced any changes in formatting that is not required by the change. Phil has made clear that he does not want any such changes in this PR, and you need to respect that. Finally, Phil has left a few comments more that I have not touched upon, so this is not all that needs to be fixed. (I left them out because I too had a hard time understanding exactly what he meant.) But if you start with these, it will be easier to see what remains. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2015000697
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:40:40 GMT, Phil Race wrote:
>> 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
>
> src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3308:
>
>> 3306: return;
>> 3307: } else {
>> 3308: pData = JNI_GET_PDATA(self);
>
> set directly
@TheShermanTanker What this means is that you need to replace the line:
PDATA pData;
above with:
AwtWindow *window;
and remove
AwtWindow *window = (AwtWindow *)pData;
below, and then change all references to `pData` in this block to `window`.
The same changes needs to be done in all places where pData is references. This
was a temporary variable that is no longer needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535500631
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v14]
On Fri, 3 Nov 2023 02:39:26 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 61: >> >>> 59: >>> 60: jfieldID AwtPrintDialog::pageID; >>> 61: >> >> where and why did this come from ? > > This came from below, all I did was move it up and out of the extern "C" > block. This cannot be inside extern "C" because it is a static class member > and has permanent C++ name mangling The question seems answered. Please resolve this discussion. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535492715
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:32:19 GMT, Phil Race wrote:
>> 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
>
> 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
Suggestion:
if (::IsWindow(hwnd))
{
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535491578
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
On Sun, 3 Dec 2023 15:37:47 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1641: >> >>> 1639: } >>> 1640: } >>> 1641: >> >> A possible improvement later (and for a future RFE) would be to use RAII for >> deletion and then get rid of the labels. awt is one of the few places that >> uses C++ for native code, so why not. > > Phil unfortunately rejected that approach, so we're on to a more manual way > of deleting things here The comment seems answered. Please resolve this discussion. >> src/java.desktop/windows/native/libawt/windows/awt_TextComponent.cpp line 59: >> >>> 57: AwtTextComponent::OleCallback AwtTextComponent::sm_oleCallback; >>> 58: WNDPROC AwtTextComponent::sm_pDefWindowProc = NULL; >>> 59: >> >> Did the compiler complain here? I'm fine with the change, just wanted to >> know the reason. > > the latter two are inside an extern "C" block, meaning their initial C++ > linkage (by virtue of them being static class members) conflicts with the now > C linkage, also the comment there states the AwtComponent fields are supposed > to be set here, and I have no idea why this was not done, so I moved them all > to be under that comment The question seem answered. Please resolve the discussion. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535492864 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535495754
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 [v50]
On Mon, 18 Mar 2024 15:55:15 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 84 commits: >> >> - Merge branch 'openjdk:master' into patch-10 >> - awt_Window.cpp >> - awt_Frame.cpp >> - awt_Component.cpp >> - awt_Canvas.cpp >> - 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 >> - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b > > bot-keep-alive > > @TheShermanTanker Did you understand the remaining changes that Phil has > requested? Do you think you'll be able to fix this? @magicus Phil doesn't seem to be responding to my queries, I'm not too sure what to do at this point - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2014826062
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 19:50:16 GMT, Phil Race wrote: >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > > See my comment > "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." > > so skip (and get rid of) the pData var and just set the target var directly @prrace Sorry for the page, but as I mentioned above I don't see much of an advantage to removing pData, since it appears we still have to do the null check anyway even without it. I'm also not sure what the incorrect code you mentioned above is - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2014317099
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Mon, 18 Mar 2024 15:55:15 GMT, Magnus Ihse Bursie wrote: > bot-keep-alive > > @TheShermanTanker Did you understand the remaining changes that Phil has > requested? Do you think you'll be able to fix this? Hi Magnus, yes I do plan on fixing this. I've just been a bit busy and tired is all, and also to be honest I don't quite understand some of Phil's reviews, which is why I'm asking for clarification on some of them first - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2008743382
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Wed, 20 Mar 2024 06:22:50 GMT, Julian Waters wrote:
>> 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;
>> }
>
> Sorry, I don't see a BOOL error anywhere?
I see the advantage of collapsing self and parent into the same check, but it
doesn't seem like getting rid of pData is of much benefit, the checks for null
seem to remain the same either way
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1531567293
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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v51]
> 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 one additional commit since the last revision: Revert Formatting in awt_PrintJob.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/0f34608b..18014c3a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=50 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=49-50 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:17:02 GMT, Phil Race wrote:
>> 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
>
> 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;
> }
Sorry, I don't see a BOOL error anywhere?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1531553778
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Sun, 21 Jan 2024 07:58:11 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 with a new target base due to a > merge or a rebase. The pull request now contains 84 commits: > > - Merge branch 'openjdk:master' into patch-10 > - awt_Window.cpp > - awt_Frame.cpp > - awt_Component.cpp > - awt_Canvas.cpp > - 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 > - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b bot-keep-alive @TheShermanTanker Did you understand the remaining changes that Phil has requested? Do you think you'll be able to fix this? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2004305924
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Sun, 21 Jan 2024 07:58:11 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 with a new target base due to a > merge or a rebase. The pull request now contains 84 commits: > > - Merge branch 'openjdk:master' into patch-10 > - awt_Window.cpp > - awt_Frame.cpp > - awt_Component.cpp > - awt_Canvas.cpp > - 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 > - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b bot-keep-alive - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1952121717
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 07:55:16 GMT, Julian Waters wrote: > Fixed the formatting (at least in the marked cases), but am unsure what you > mean by set directly? See my comment "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." so skip (and get rid of) the pData var and just set the target var directly - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1902742055
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
> 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 one additional commit since the last revision: awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/ffe5ef13..470c8bb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=48 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=47-48 Stats: 15 lines in 1 file changed: 5 ins; 0 del; 10 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
> 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 84 commits: - Merge branch 'openjdk:master' into patch-10 - awt_Window.cpp - awt_Frame.cpp - awt_Component.cpp - awt_Canvas.cpp - 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 - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b - Changes: https://git.openjdk.org/jdk/pull/15096/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=49 Stats: 590 lines in 12 files changed: 461 ins; 30 del; 99 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 07:55:13 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 one additional > commit since the last revision: > > awt_Window.cpp Fixed the formatting (at least in the marked cases), but am unsure what you mean by set directly? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1902545943
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v48]
> 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 one additional commit since the last revision: awt_Frame.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/07b00de2..ffe5ef13 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=47 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=46-47 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v47]
> 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: - awt_Component.cpp - awt_Canvas.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/cbfbaee6..07b00de2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=46 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=45-46 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Thu, 11 Jan 2024 08:24:42 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 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/liba
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v26]
On Thu, 16 Nov 2023 03:44:53 GMT, Phil Race wrote: >> I happened to ask around on the build-dev mailing lists about whether we >> include msvcp.dll with the JDK, here is Erik's response: >> >>> Back in JDK 8 when we used Visual Studio 2010, we used to not ship >>> msvcp*.dll. This changed when I added support for building with Visual >>> Studio 2013 [1] in JDK 9. In the patch for that bug I found this text: >>> >>>+ # If building with Visual Studio 2010, we can still use >>> _STATIC_CPPLIB to >>>+ # avoid bundling msvcpNNN.dll. Doesn't work with newer versions of >>> visual >>>+ # studio. >>> >>> So since we switched to Visual Studio 2013, we started to bundle >>> msvcp*.dll. It was only ever possible to not bundle it if you built with >>> Visual Studio 2010 (or older I suppose). It's pretty safe to say that >>> the current mainline JDK build requires msvcp.dll to be bundled. Looking >>> at the configure logic, configure will fail unless we find it and the >>> copying in open/make/modules/java.base/Copy.gmk is unconditional. >>> >>> /Erik >>> >>> [1] https://bugs.openjdk.org/browse/JDK-8042707 >> >> This may be unrelated, but I really think we should kill the std::bad_alloc >> hack in awt.dll as such > >> I happened to ask around on the build-dev mailing lists about whether we >> include msvcp.dll with the JDK, here is Erik's response: > > Yes, I saw that. It doesn't make any difference. > Although BTW the comment points out we shouldn't be doing static linking > anymore. > > >> This may be unrelated, but I really think we should kill the std::bad_alloc >> hack in awt.dll as such > > seems unrelated, yes, but I'd have to look at that, to see if there's a > reason to touch it. Itchy fingers are not a valid reason. @prrace ? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1893138028
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Thu, 11 Jan 2024 12:11:17 GMT, Magnus Ihse Bursie wrote: > What is remaining to get this PR committable? It has such a long history that > it is hard to get a grasp on the remaining issues. > > @TheShermanTanker Could you perhaps summarize the remaining hurdles? It's largely complete by now, just waiting for approval from @prrace for the awt parts of the codebase. The vast majority of this is just hand-inlining macros in awt to avoid jumping across locals with gotos, it's very messy on the surface, sorry about that :( And of course, as soon as I say that I get a mass JTReg failure that makes nearly every run on GHA turn red, thanks for the heart attack JTReg - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1887046380
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Thu, 11 Jan 2024 08:24:42 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 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 What is remaining to get this PR committable? It has such a long history that it is hard to get a grasp on the remaining issues. @TheShermanTanker Could you perhaps summarize the remaining hurdles? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1887030877
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 09:39:09 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 with a new target base due to a > merge or a rebase. The pull request now contains 78 commits: > > - 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 > - awt_Frame.cpp > - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f Bumping - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1886600273
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
> 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: https://git.openjdk.org/jdk/pull/15096/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=45 Stats: 615 lines in 12 files changed: 461 ins; 38 del; 116 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 09:39:09 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 with a new target base due to a > merge or a rebase. The pull request now contains 78 commits: > > - 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 > - awt_Frame.cpp > - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f Do not. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1873418448
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 11:33:09 GMT, Julian Waters wrote: >> make/autoconf/flags-cflags.m4 line 565: >> >>> 563: # The -utf-8 option sets source and execution character sets to >>> UTF-8 to enable correct >>> 564: # compilation of all source files regardless of the active code >>> page on Windows. >>> 565: TOOLCHAIN_CFLAGS_JVM="-nologo -MD -Zc:preprocessor >>> -Zc:strictStrings -Zc:inline -permissive- -utf-8 -MP" >> >> What is the rationale for removing `-Zc:strictStrings`? That seems like a >> step backwards. Also, this will affect *all* files compiled, both hotspot >> and all native JDK libraries. >> >> If there is a single file that cannot (for some reason) be fixed to have the >> compiler stop complaining about const strings, that individual file should >> have `-Zc:strictStrings-` added to its CFLAGS. > > @magicus -permissive- automatically turns on -Zc:strictStrings, so specifying > it manually becomes redundant when -permissive- is also specified > https://learn.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170 > >> The -Zc:strictStrings option is off by default. The >> [-permissive-](https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170) >> compiler option implicitly sets this option, but it can be overridden by >> using -Zc:strictStrings- I see. Then I guess this is okay. If/when you are finally ready for integration, and turning on -permissive-, let me know so I can take the patch for a spin in our CI systems before integration. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1413756555
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 11:29:07 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 78 commits: >> >> - 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 >> - awt_Frame.cpp >> - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f > > make/autoconf/flags-cflags.m4 line 565: > >> 563: # The -utf-8 option sets source and execution character sets to >> UTF-8 to enable correct >> 564: # compilation of all source files regardless of the active code >> page on Windows. >> 565: TOOLCHAIN_CFLAGS_JVM="-nologo -MD -Zc:preprocessor >> -Zc:strictStrings -Zc:inline -permissive- -utf-8 -MP" > > What is the rationale for removing `-Zc:strictStrings`? That seems like a > step backwards. Also, this will affect *all* files compiled, both hotspot and > all native JDK libraries. > > If there is a single file that cannot (for some reason) be fixed to have the > compiler stop complaining about const strings, that individual file should > have `-Zc:strictStrings-` added to its CFLAGS. @magicus -permissive- automatically turns on -Zc:strictStrings, so specifying it manually becomes redundant when -permissive- is also specified https://learn.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170 > The -Zc:strictStrings option is off by default. The > [-permissive-](https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170) > compiler option implicitly sets this option, but it can be overridden by > using -Zc:strictStrings- - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1413745163
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 09:39:09 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 with a new target base due to a > merge or a rebase. The pull request now contains 78 commits: > > - 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 > - awt_Frame.cpp > - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f make/autoconf/flags-cflags.m4 line 565: > 563: # The -utf-8 option sets source and execution character sets to > UTF-8 to enable correct > 564: # compilation of all source files regardless of the active code page > on Windows. > 565: TOOLCHAIN_CFLAGS_JVM="-nologo -MD -Zc:preprocessor -Zc:strictStrings > -Zc:inline -permissive- -utf-8 -MP" What is the rationale for removing `-Zc:strictStrings`? That seems like a step backwards. Also, this will affect *all* files compiled, both hotspot and all native JDK libraries. If there is a single file that cannot (for some reason) be fixed to have the compiler stop complaining about const strings, that individual file should have `-Zc:strictStrings-` added to its CFLAGS. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1413741134
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 09:39:09 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 with a new target base due to a > merge or a rebase. The pull request now contains 78 commits: > > - 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 > - awt_Frame.cpp > - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f Re-adding build label since this now touches build flags again - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1838179902
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v8]
> 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 one additional commit since the last revision: Don't use permissive- in flags-cflags.m4 for now - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/15ae2781..0566de0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=06-07 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v7]
On Tue, 10 Oct 2023 03:44:27 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 five additional > commits since the last revision: > > - Revert sspi.cpp > - Revert NativeCreds.c > - Revert allocation.cpp > - Revert symbolengine.cpp > - Revert os_windows.cpp > This got a bit messy. In hindsight it would probably have been better to > leave this closed and open a new bug. But what's done is done, let's not mess > around any more at this point. > > If all other places where `permissive-` failed are fixed, then I think it is > okay to include the flag in this PR. Otherwise, you cannot do that. As a > general rule, changing compiler options is best done in a separate patch, to > differentiate any potential bad effect from actual code changes. However, in > this case, I don't think the flag will cause any differences in the generated > code, only in what syntax the compiler accepts, so I guess it is fine. > > /reviewers 2 Sorry I was busy with work on my end, I'm not planning to integrate -permissive- in this Pull Request, I must have missed it. I'll remove it soon - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1760636258
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v7]
On Tue, 10 Oct 2023 03:44:27 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 five additional > commits since the last revision: > > - Revert sspi.cpp > - Revert NativeCreds.c > - Revert allocation.cpp > - Revert symbolengine.cpp > - Revert os_windows.cpp This got a bit messy. In hindsight it would probably have been better to leave this closed and open a new bug. But what's done is done, let's not mess around any more at this point. If all other places where `permissive-` failed are fixed, then I think it is okay to include the flag in this PR. Otherwise, you cannot do that. As a general rule, changing compiler options is best done in a separate patch, to differentiate any potential bad effect from actual code changes. However, in this case, I don't think the flag will cause any differences in the generated code, only in what syntax the compiler accepts, so I guess it is fine. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1759347530
