8304717: Declaration aliasing between boolean and jboolean is wrong #13139

TheShermanTanker · 2023-03-22T14:02:53Z

A couple of spots wrongly refer to boolean and jboolean as the same thing. While this does still compile thanks to a happy accident and implicit conversions, they are not the same at all, and should be fixed before a future compiler error happens if their declarations are touched

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304717: Declaration aliasing between boolean and jboolean is wrong

Reviewers

David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13139/head:pull/13139
$ git checkout pull/13139

Update a local copy of the PR:
$ git checkout pull/13139
$ git pull https://git.openjdk.org/jdk.git pull/13139/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13139

View PR using the GUI difftool:
$ git pr show -t 13139

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13139.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-22T14:03:57Z

👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-22T14:06:01Z

@TheShermanTanker The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-22T14:19:47Z

Webrevs

dholmes-ora

One change has an issue but the others seem fine. You really need reviewers from each component area though.

Thanks.

dholmes-ora · 2023-03-24T03:02:47Z

src/java.prefs/macosx/native/libprefs/MacOSXPreferencesFile.m

@@ -682,7 +682,7 @@ static void createTreeForPath(CFStringRef path, CFStringRef name,
    CFDictionaryRef node;
    CFStringRef topKey;
    CFMutableDictionaryRef topValue;
-    Boolean beforeAdd = false;


The return value from CFDictionaryContainsKey is a Boolean and is assigned to this variable. So I think these changes are the wrong way round. Keep this as a Boolean but convert the return value to jboolean:

return beforeAdd ? JNI_TRUE : JNI_FALSE;

Argh, looks like Apple and Objective-C still use the ancient Boolean types from Carbon for the CoreFoundation APIs. Sigh...

dholmes-ora · 2023-03-24T03:05:46Z

/label add awt

Adding AWT folk.

openjdk · 2023-03-24T03:07:47Z

@dholmes-ora
The label awt is not a valid label.
These labels are valid:

serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

TheShermanTanker · 2023-03-24T04:09:51Z

Correct label for awt is client to my knowledge

TheShermanTanker · 2023-03-24T04:09:58Z

/label client

openjdk · 2023-03-24T04:10:22Z

@TheShermanTanker
The client label was successfully added.

dholmes-ora · 2023-03-24T05:07:57Z

Thanks for the correction I thought the mailing list was awt-dev similar to swing-dev.

merykitty · 2023-03-24T05:34:51Z

Is this needed? A boolean-to-int conversion returns 1 if the input is true and 0 otherwise. The opposite returns true for non-zero value and false otherwise. Unless we have some tricky jboolean value that should not happen they should behave the same?

TheShermanTanker · 2023-03-24T14:47:43Z

I'd argue it's more for correctness: Indeed it will still compile fine as of now, but the type of jboolean is not the only thing that can change; the other typedef'd boolean type can also be modified later on. Additionally, with compilers getting stricter as newer releases drop, it's a good idea to make sure the signatures match as best as possible, especially between declarations and definitions of the same method

dholmes-ora · 2023-03-31T06:33:09Z

src/java.prefs/macosx/native/libprefs/MacOSXPreferencesFile.m

@@ -695,9 +695,9 @@ static void createTreeForPath(CFStringRef path, CFStringRef name,
    beforeAdd = CFDictionaryContainsKey(parent, child);
    CFDictionaryAddValue(parent, child, node);
    if (!beforeAdd)
-        beforeAdd = CFDictionaryContainsKey(parent, child);
+        beforeAdd = CFDictionaryContainsKey(parent, child) ? JNI_TRUE : JNI_FALSE;


If you do this here you need something similar on line 695. Still say it is simpler to use Boolean internally and convert to jboolean on return expression.

prrace

The single java.desktop change looks OK, so when anyone approves the rest that's implicitly approved.

TheShermanTanker · 2023-04-08T13:21:01Z

Alright, thanks Phil

dholmes-ora

Okay. Thanks.

openjdk · 2023-04-11T03:14:23Z

@TheShermanTanker This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304717: Declaration aliasing between boolean and jboolean is wrong

Reviewed-by: dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 235 new commits pushed to the master branch:

1375130: 8305728: RISC-V: Use bexti instruction to do single-bit testing
4485737: 8304501: Remove orphaned demo netbeans projects
0ff2ff6: 8305807: Spurious right brace in ConstantDescs field Javadocs
2aeb0e5: 6241286: (cal) API: Calendar.DAY_OF_WEEK definition is wrong
42965d3: 6218123: (cal) API: Spec for GregorianCalendar constructors and Calendar getInstance is inconsistent.
a43a17c: 8305808: Typo in javadoc of ConstantDescs::BSM_VARHANDLE_STATIC_FIELD
ba90dc7: 8304911: Use OperatingSystem enum in some modules
76975da: 8305758: Update the JAR tool man page to indicate -i/--generate-file is deprecated
0d45a52: 8302819: Remove JAR Index
0243da2: 8305481: gtest is_first_C_frame failing on ARM
... and 225 more: https://git.openjdk.org/jdk/compare/ddf1e34c1a0815e8677212f1a7860ca7cf9fc2c9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

TheShermanTanker · 2023-04-11T06:49:30Z

Thanks David!

TheShermanTanker · 2023-04-11T06:51:09Z

/integrate

openjdk · 2023-04-11T06:52:44Z

Going to push as commit cd7d53c.
Since your change was applied there have been 237 commits pushed to the master branch:

2586f36: 8304919: Implementation of Virtual Threads
3939807: 8182621: JSSE should reject empty TLS plaintexts
1375130: 8305728: RISC-V: Use bexti instruction to do single-bit testing
4485737: 8304501: Remove orphaned demo netbeans projects
0ff2ff6: 8305807: Spurious right brace in ConstantDescs field Javadocs
2aeb0e5: 6241286: (cal) API: Calendar.DAY_OF_WEEK definition is wrong
42965d3: 6218123: (cal) API: Spec for GregorianCalendar constructors and Calendar getInstance is inconsistent.
a43a17c: 8305808: Typo in javadoc of ConstantDescs::BSM_VARHANDLE_STATIC_FIELD
ba90dc7: 8304911: Use OperatingSystem enum in some modules
76975da: 8305758: Update the JAR tool man page to indicate -i/--generate-file is deprecated
... and 227 more: https://git.openjdk.org/jdk/compare/ddf1e34c1a0815e8677212f1a7860ca7cf9fc2c9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-11T06:52:54Z

@TheShermanTanker Pushed as commit cd7d53c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jboolean

a683d7f

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 22, 2023

awt_PrintJob.cpp

0df005b

TheShermanTanker changed the title ~~WIP~~ 8304717 Mar 22, 2023

openjdk bot changed the title ~~8304717~~ 8304717: Declaration aliasing between boolean and jboolean is wrong Mar 22, 2023

openjdk bot added the rfr Pull request is ready for review label Mar 22, 2023

MacOSXPreferencesFile.m

3f9da8b

dholmes-ora suggested changes Mar 24, 2023

View reviewed changes

openjdk bot added the client client-libs-dev@openjdk.org label Mar 24, 2023

Explicit conversion to jboolean

362cf3e

dholmes-ora suggested changes Mar 31, 2023

View reviewed changes

prrace reviewed Apr 7, 2023

View reviewed changes

Legacy Boolean to jboolean conversion

dad94b3

dholmes-ora approved these changes Apr 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 11, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 11, 2023

openjdk bot closed this Apr 11, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 11, 2023

TheShermanTanker deleted the patch-2 branch April 11, 2023 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8304717: Declaration aliasing between boolean and jboolean is wrong #13139

8304717: Declaration aliasing between boolean and jboolean is wrong #13139

TheShermanTanker commented Mar 22, 2023 •

edited by openjdk bot

bridgekeeper bot commented Mar 22, 2023

openjdk bot commented Mar 22, 2023

mlbridge bot commented Mar 22, 2023 •

edited

dholmes-ora left a comment

dholmes-ora Mar 24, 2023

TheShermanTanker Mar 31, 2023

dholmes-ora commented Mar 24, 2023 •

edited

openjdk bot commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

dholmes-ora commented Mar 24, 2023

merykitty commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

dholmes-ora Mar 31, 2023

prrace left a comment

TheShermanTanker commented Apr 8, 2023

dholmes-ora left a comment

openjdk bot commented Apr 11, 2023

TheShermanTanker commented Apr 11, 2023

TheShermanTanker commented Apr 11, 2023

openjdk bot commented Apr 11, 2023

openjdk bot commented Apr 11, 2023

8304717: Declaration aliasing between boolean and jboolean is wrong #13139

8304717: Declaration aliasing between boolean and jboolean is wrong #13139

Conversation

TheShermanTanker commented Mar 22, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 22, 2023

openjdk bot commented Mar 22, 2023

mlbridge bot commented Mar 22, 2023 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Mar 24, 2023

Choose a reason for hiding this comment

TheShermanTanker Mar 31, 2023

Choose a reason for hiding this comment

dholmes-ora commented Mar 24, 2023 • edited

openjdk bot commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

dholmes-ora commented Mar 24, 2023

merykitty commented Mar 24, 2023

TheShermanTanker commented Mar 24, 2023

dholmes-ora Mar 31, 2023

Choose a reason for hiding this comment

prrace left a comment

Choose a reason for hiding this comment

TheShermanTanker commented Apr 8, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 11, 2023

TheShermanTanker commented Apr 11, 2023

TheShermanTanker commented Apr 11, 2023

openjdk bot commented Apr 11, 2023

openjdk bot commented Apr 11, 2023

TheShermanTanker commented Mar 22, 2023 •

edited by openjdk bot

mlbridge bot commented Mar 22, 2023 •

edited

dholmes-ora commented Mar 24, 2023 •

edited