Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 02:38:13 GMT, Julian Waters wrote: >> On Windows, the basic Java Integer types are defined as long and __int64 >> respectively. In particular, the former is rather problematic since it >> breaks compilation as the Visual C++ becomes stricter and more compliant >> with every release, which means the way Windows code treats long as a >> typedef for int is no longer correct, especially with -permissive- enabled. >> Instead of changing every piece of broken code to match the jint = long >> typedef, which is far too time consuming, we can instead change jint to an >> int (which is still the same 32 bit number type as long), as there are far >> fewer problems caused by this definition. It's better to get this over and >> done with sooner than later when a future version of Visual C++ finally >> starts to break on existing code > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert wrong Copyright > - Copyright Alright, waiting for you to do the honours :) - PR Comment: https://git.openjdk.org/jdk/pull/14125#issuecomment-1604563651
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 16:53:01 GMT, Alexey Ivanov wrote: >> Hmm, I lean towards jint as I feel it conveys the fact that it is a Java >> parameter clearer, intuitively to me it makes sense that a Java integer type >> would still work in a C++ for loop in native code > > You're right… it gives a hint it'll be an upcall into Java. Let's go for > `jint` then. > > I don't think there's a need to change the type of the for-loop variable to > `jint`. Oh no, I didn't mean to change the loop variable, rather that leaving the jint as is should be fine in the for loop - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1240044495
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 14:30:49 GMT, Julian Waters wrote: >> To minimise the number of changes, we can go for using `jint` in >> `AwtMenu::GetItem`. >> >> What do you thing, @djelinski and @TheShermanTanker? > > Hmm, I lean towards jint as I feel it conveys the fact that it is a Java > parameter clearer, intuitively to me it makes sense that a Java integer type > would still work in a C++ for loop in native code You're right… it gives a hint it'll be an upcall into Java. Let's go for `jint` then. I don't think there's a need to change the type of the for-loop variable to `jint`. - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1240041248
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 14:28:51 GMT, Alexey Ivanov wrote: >> The declaration and implementation have to match. > > To minimise the number of changes, we can go for using `jint` in > `AwtMenu::GetItem`. > > What do you thing, @djelinski and @TheShermanTanker? Hmm, I lean towards jint as I feel it conveys the fact that it is a Java parameter clearer, intuitively to me it makes sense that a Java integer type would still work in a C++ for loop in native code - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1239893754
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 14:24:44 GMT, Alexey Ivanov wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Menu.h line 76: >> >>> 74: /*for multifont menu */ >>> 75: BOOL IsTopMenu(); >>> 76: virtual AwtMenuItem* GetItem(jobject target, int index); >> >> Hi @aivanov-jdk are you OK leaving this inconsistent with the definition? >> https://github.com/openjdk/jdk/blob/16b5a91461db1765e2e7596ebaaf1299cec9b0c8/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp#L261 > > The declaration and implementation have to match. To minimise the number of changes, we can go for using `jint` in `AwtMenu::GetItem`. What do you thing, @djelinski and @TheShermanTanker? - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1239891473
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 06:10:05 GMT, Daniel Jeliński wrote: >> Julian Waters has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Revert wrong Copyright >> - Copyright > > src/java.desktop/windows/native/libawt/windows/awt_Menu.h line 76: > >> 74: /*for multifont menu */ >> 75: BOOL IsTopMenu(); >> 76: virtual AwtMenuItem* GetItem(jobject target, int index); > > Hi @aivanov-jdk are you OK leaving this inconsistent with the definition? > https://github.com/openjdk/jdk/blob/16b5a91461db1765e2e7596ebaaf1299cec9b0c8/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp#L261 The declaration and implementation have to match. - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1239886709
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
On Fri, 23 Jun 2023 02:38:13 GMT, Julian Waters wrote: >> On Windows, the basic Java Integer types are defined as long and __int64 >> respectively. In particular, the former is rather problematic since it >> breaks compilation as the Visual C++ becomes stricter and more compliant >> with every release, which means the way Windows code treats long as a >> typedef for int is no longer correct, especially with -permissive- enabled. >> Instead of changing every piece of broken code to match the jint = long >> typedef, which is far too time consuming, we can instead change jint to an >> int (which is still the same 32 bit number type as long), as there are far >> fewer problems caused by this definition. It's better to get this over and >> done with sooner than later when a future version of Visual C++ finally >> starts to break on existing code > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert wrong Copyright > - Copyright src/java.desktop/windows/native/libawt/windows/awt_Menu.h line 76: > 74: /*for multifont menu */ > 75: BOOL IsTopMenu(); > 76: virtual AwtMenuItem* GetItem(jobject target, int index); Hi @aivanov-jdk are you OK leaving this inconsistent with the definition? https://github.com/openjdk/jdk/blob/16b5a91461db1765e2e7596ebaaf1299cec9b0c8/src/java.desktop/windows/native/libawt/windows/awt_Menu.cpp#L261 - PR Review Comment: https://git.openjdk.org/jdk/pull/14125#discussion_r1239358334
Re: RFR: 8308780: Fix the Java Integer types on Windows [v11]
> On Windows, the basic Java Integer types are defined as long and __int64 > respectively. In particular, the former is rather problematic since it breaks > compilation as the Visual C++ becomes stricter and more compliant with every > release, which means the way Windows code treats long as a typedef for int is > no longer correct, especially with -permissive- enabled. Instead of changing > every piece of broken code to match the jint = long typedef, which is far too > time consuming, we can instead change jint to an int (which is still the same > 32 bit number type as long), as there are far fewer problems caused by this > definition. It's better to get this over and done with sooner than later when > a future version of Visual C++ finally starts to break on existing code Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Revert wrong Copyright - Copyright - Changes: - all: https://git.openjdk.org/jdk/pull/14125/files - new: https://git.openjdk.org/jdk/pull/14125/files/80b6f787..16b5a914 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14125=10 - incr: https://webrevs.openjdk.org/?repo=jdk=14125=09-10 Stats: 7 lines in 7 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/14125.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14125/head:pull/14125 PR: https://git.openjdk.org/jdk/pull/14125
