8312121: Fix -Wconversion warnings in tribool.hpp #14892

coleenp · 2023-07-14T19:47:39Z

Assigning _value first, and then doing _value | 2 doesn't get -Wconversion warnings. Also, reduced include file inclusion a little.
Tested with tier1 on linux-x64-debug, windows-x64-debug, macos-aarch64-debug

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-8312121: Fix -Wconversion warnings in tribool.hpp (Enhancement - P4)

Reviewers

Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 73c2075a
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14892

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-14T19:49:07Z

👋 Welcome back coleenp! 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-07-14T19:50:07Z

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

hotspot

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-07-14T21:28:30Z

Webrevs

dean-long · 2023-07-15T01:30:49Z

src/hotspot/share/utilities/tribool.hpp

@@ -40,11 +40,14 @@ class TriBool{

 public:
  TriBool() : _value(0) {}
-  TriBool(bool value) : _value(((u1)value) | 2) {}
+  TriBool(bool value) : _value(value) {
+    _value = _value | 2;


You can also do like line 39 and use (u1)(expr) & 3 or ((expr & 3u) also seems to work.

Or cast the 2 to (u1)2 ?

It does seem strange that this code is using a bit-field, but still using & | and shift. Why not represent the high and low bit with two separate bit-fields of width 1?

I tried to change the class to be int value : 1, init: 1; the changes got a bit ugly when fixing the TriBoolArray functions.

expr & 3 is the wrong expression. This wants to set the 'not-default' bit, not and in the 'not-default' bit if it's set in the value. Thankfully, tribool has a gtest.

tribool.hpp:43:43: warning: conversion from 'unsigned int' to 'unsigned char:2' may change value [-Wconversion] 43 | TriBool(bool value) : _value((u1)value | (u1)2) { | ~~~~~~~~~~~^~~~~~~

Casting 2 to u1 doesn't work because | still promotes to unsigned int.

openjdk · 2023-07-15T01:31:57Z

@coleenp 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:

8312121: Fix -Wconversion warnings in tribool.hpp

Reviewed-by: dlong, 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 184 new commits pushed to the master branch:

c27c877: 8302017: Allocate BadPaddingException only if it will be thrown
c55d29f: 8312626: Resolve multiple definition of 'start_timer' when statically linking JDK native libraries with user code
0ca2bfd: 8311104: dangling-gsl warning in libwixhelper.cpp
c05ba48: 8313250: Exclude java/foreign/TestByteBuffer.java on AIX
169b6e3: 8313174: Create fewer predictable port clashes in management tests
8650026: 8310033: Clarify return value of Java Time compareTo methods
25058cd: 8312620: WSL Linux build crashes after JDK-8310233
8661b8e: 8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms
486c784: 8312433: HttpClient request fails due to connection being considered idle and closed
271417a: 8312579: [JVMCI] JVMCI support for virtual Vector API objects
... and 174 more: https://git.openjdk.org/jdk/compare/fd7fddb6ed24e0ae4b5e18fb27fd80230941dc0b...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.

dholmes-ora

Okay. Beats me why the compiler can't tell that the result of the OR fits in two bits without having to do the & 3.

Thanks

coleenp · 2023-07-25T12:22:49Z

This doesn't compile on Windows, without a whole lot of other casts. I like my solution better now. Or I'll throw in more u1 casts. 🤢

dholmes-ora

To me the fact we have so much trouble placating the compilers suggests that we are doing something wrong/broken here! :(

Is the & 3 still needed with the extra u1 cast? I'm gonna guess yes as a u1 doesn't fit in a uchar:2 any better than a uint does.

dean-long · 2023-07-26T04:31:11Z

We don't need the & 3 if we do something like:

_value = newval ? 3 : 2;

dean-long · 2023-07-26T04:48:42Z

Is the & 3 still needed with the extra u1 cast? I'm gonna guess yes as a u1 doesn't fit in a uchar:2 any better than a uint does.

Good question. I got this to work:

_value = ((u1)newval | 2u);

but it requires gcc 11.3.

dholmes-ora · 2023-07-26T05:32:43Z

We don't need the & 3 if we do something like:

_value = newval ? 3 : 2;

That seems a lot better/clearer than casts and bit-wise operations.

dean-long · 2023-07-26T07:33:48Z

I like this version:
_value = (newval ? 1 : 0) | 2;

coleenp · 2023-07-27T12:56:15Z

My first version did the explicit | 2 separately and if you don't like all the casts, I think that's the most explicit version. It sets the value, then sets the top bit that says it's not the default value.

dean-long · 2023-07-27T19:17:23Z

V1 looks good and seems to produce identical code.

coleenp · 2023-07-27T20:13:15Z

Thanks, Dean.

dholmes-ora

Maybe somewhere a comment could be added like:

// Update _value in two steps to avoid awkward casts needed to silence conversion warnings

otherwise someone is bound to rewrite those two lines as one at some point in the future.

Thanks.

dholmes-ora · 2023-07-27T22:26:13Z

src/hotspot/share/utilities/tribool.hpp

@@ -40,11 +40,14 @@ class TriBool{

 public:
  TriBool() : _value(0) {}
-  TriBool(bool value) : _value(((u1)value) | 2) {}
+  TriBool(bool value) : _value(value) {
+    _value = _value | 2; // set to not-default


Why not |= here?

I was trying different versions to see if |= would give a warning.

dean-long · 2023-07-28T03:51:43Z

This version still looks good. If you want to do the |= in only one place, you could do it in TriBool operator = and have the other two places use TriBool operator =.

dean-long · 2023-07-28T03:53:29Z

src/hotspot/share/utilities/tribool.hpp

+  TriBool(bool value) : _value(value) {
+    // set to not-default in separate step to avoid conversion warnings
+    _value |= 2;


Suggested change

TriBool(bool value) : _value(value) {

// set to not-default in separate step to avoid conversion warnings

_value |= 2;

TriBool(bool value) {

*this = value;

Thanks for the suggestion but this makes me squint to try to figure out what it does and why it's correct.

dean-long · 2023-07-28T03:54:23Z

src/hotspot/share/utilities/tribool.hpp

+      _value = newval;
+      _value |= 2; // set to not-default


Suggested change

_value = newval;

_value |= 2; // set to not-default

TriBool::operator=(newval);

I don't think this is more readable. I believe you that it doesn't give -Wconversion warnings though.

coleenp · 2023-07-28T12:07:21Z

Thanks for all the work reviewing Dean and David.
/integrate

openjdk · 2023-07-28T12:08:26Z

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

a3d6723: 8311033: [macos] PrinterJob does not take into account Sides attribute
4ae5a3e: 8306446: java/lang/management/ThreadMXBean/Locks.java transient failures
cad6114: 8304954: SegmentedCodeCache fails when using large pages
ba645da: 8313082: Enable CreateCoredumpOnCrash for testing in makefiles
c27c877: 8302017: Allocate BadPaddingException only if it will be thrown
c55d29f: 8312626: Resolve multiple definition of 'start_timer' when statically linking JDK native libraries with user code
0ca2bfd: 8311104: dangling-gsl warning in libwixhelper.cpp
c05ba48: 8313250: Exclude java/foreign/TestByteBuffer.java on AIX
169b6e3: 8313174: Create fewer predictable port clashes in management tests
8650026: 8310033: Clarify return value of Java Time compareTo methods
... and 178 more: https://git.openjdk.org/jdk/compare/fd7fddb6ed24e0ae4b5e18fb27fd80230941dc0b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-28T12:08:33Z

@coleenp Pushed as commit 47c4b99.

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

8312121: Fix -Wconversion warnings in tribool.hpp

73c2075

openjdk bot added the hotspot label Jul 14, 2023

coleenp marked this pull request as ready for review July 14, 2023 21:22

openjdk bot added the rfr label Jul 14, 2023

dean-long reviewed Jul 15, 2023

View reviewed changes

dean-long approved these changes Jul 15, 2023

View reviewed changes

openjdk bot added the ready label Jul 15, 2023

coleenp added 2 commits July 25, 2023 02:27

Dean's suggestion.

c0e12b5

Missed TriBoolAssigner case.

f4f32fb

dholmes-ora approved these changes Jul 25, 2023

View reviewed changes

Add some more u1 casts to keep Windows compiler happy hopefully.

f135916

dholmes-ora approved these changes Jul 26, 2023

View reviewed changes

Back to V1.

d380e8e

dholmes-ora approved these changes Jul 27, 2023

View reviewed changes

coleenp added 2 commits July 27, 2023 22:42

David suggestion

d17c7a5

David suggestion

288b47c

dholmes-ora approved these changes Jul 27, 2023

View reviewed changes

dean-long reviewed Jul 28, 2023

View reviewed changes

openjdk bot added the integrated label Jul 28, 2023

openjdk bot closed this Jul 28, 2023

openjdk bot removed ready rfr labels Jul 28, 2023

coleenp deleted the tribool branch July 28, 2023 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8312121: Fix -Wconversion warnings in tribool.hpp #14892

8312121: Fix -Wconversion warnings in tribool.hpp #14892

coleenp commented Jul 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

mlbridge bot commented Jul 14, 2023 •

edited

Loading

dean-long Jul 15, 2023

dholmes-ora Jul 17, 2023

dean-long Jul 17, 2023

coleenp Jul 24, 2023

coleenp Jul 24, 2023

openjdk bot commented Jul 15, 2023 •

edited

Loading

dholmes-ora left a comment

coleenp commented Jul 25, 2023 •

edited

Loading

dholmes-ora left a comment

dean-long commented Jul 26, 2023

dean-long commented Jul 26, 2023

dholmes-ora commented Jul 26, 2023

dean-long commented Jul 26, 2023

coleenp commented Jul 27, 2023

dean-long commented Jul 27, 2023

coleenp commented Jul 27, 2023

dholmes-ora left a comment

dholmes-ora Jul 27, 2023

coleenp Jul 27, 2023

dean-long commented Jul 28, 2023

dean-long Jul 28, 2023

coleenp Jul 28, 2023

dean-long Jul 28, 2023

coleenp Jul 28, 2023

coleenp commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

	_value = newval;
	_value \|= 2; // set to not-default
	TriBool::operator=(newval);

8312121: Fix -Wconversion warnings in tribool.hpp #14892

8312121: Fix -Wconversion warnings in tribool.hpp #14892

Conversation

coleenp commented Jul 14, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 14, 2023

openjdk bot commented Jul 14, 2023

mlbridge bot commented Jul 14, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 15, 2023 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Jul 25, 2023 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

dean-long commented Jul 26, 2023

dean-long commented Jul 26, 2023

dholmes-ora commented Jul 26, 2023

dean-long commented Jul 26, 2023

coleenp commented Jul 27, 2023

dean-long commented Jul 27, 2023

coleenp commented Jul 27, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dean-long commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

coleenp commented Jul 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 14, 2023 •

edited

Loading

openjdk bot commented Jul 15, 2023 •

edited

Loading

coleenp commented Jul 25, 2023 •

edited

Loading