Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore wrote: >> Please review change for mostly fixing return types in the constant pool and >> metadata to fix -Wconversion warnings in JVMTI code. The order of >> preference for changes are: 1. change the types to more distinct types >> (fields in the constant pool are u2 because that's their size in the >> classfile), 2. add direct int casts if the value has been checked in asserts >> above, and 3. checked_cast<> if not verified, and 4. added some >> pointer_delta_as_ints where needed. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > David's suggestions. Thanks David for the review. I do change the types as low in the call stack as practical with these changes. I think your comment above disappeared because the last change reverted that code. - PR Comment: https://git.openjdk.org/jdk/pull/14710#issuecomment-1621658684
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On 3/07/2023 10:55 am, David Holmes wrote: On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore wrote: Please review change for mostly fixing return types in the constant pool and metadata to fix -Wconversion warnings in JVMTI code. The order of preference for changes are: 1. change the types to more distinct types (fields in the constant pool are u2 because that's their size in the classfile), 2. add direct int casts if the value has been checked in asserts above, and 3. checked_cast<> if not verified, and 4. added some pointer_delta_as_ints where needed. Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: David's suggestions. src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 92: 90: result &= small_offset_mask; // cut off the hash bits 91: } 92: return result; Doesn't this trigger a warning as we go from unsigned to signed? Weird: I was sure I deleted this comment. Please ignore. I can't even find it in the PR so responding via email. David - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1249960932
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore wrote: >> Please review change for mostly fixing return types in the constant pool and >> metadata to fix -Wconversion warnings in JVMTI code. The order of >> preference for changes are: 1. change the types to more distinct types >> (fields in the constant pool are u2 because that's their size in the >> classfile), 2. add direct int casts if the value has been checked in asserts >> above, and 3. checked_cast<> if not verified, and 4. added some >> pointer_delta_as_ints where needed. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > David's suggestions. As a general rule I'd prefer to see the lowest-level functions use the types of the actual thing they are exposing - so for anything CP related we return u1/u2/u4 - and then have higher-level code do any convenience conversions. But I realize this is tricky to deal with and there are alternatives and trade-offs in where these warnings get silenced. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1510139930
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore wrote: >> Please review change for mostly fixing return types in the constant pool and >> metadata to fix -Wconversion warnings in JVMTI code. The order of >> preference for changes are: 1. change the types to more distinct types >> (fields in the constant pool are u2 because that's their size in the >> classfile), 2. add direct int casts if the value has been checked in asserts >> above, and 3. checked_cast<> if not verified, and 4. added some >> pointer_delta_as_ints where needed. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > David's suggestions. src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 92: > 90: result &= small_offset_mask; // cut off the hash bits > 91: } > 92: return result; Doesn't this trigger a warning as we go from unsigned to signed? - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1249960932
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
On Fri, 30 Jun 2023 02:11:11 GMT, David Holmes wrote: >> I had to change these two lines because BytecodeStream::get_index_u2 returns >> an int, so got the warning and this didn't need to be declared with u2. >> get_index_u2() could be fixed to return a u2 but I didn't want to go that >> far as no casts were involved in this change. > > I think this change looks "wrong" - the indices are supposed to be u2's, if > the function returns an int that seems an error. Fixing these functions will have fallout further down in the code, so I didn't want to do this. These functions are used in a lot of places in the code where int is used for convenience. int is a convenient type. This didn't require a cast because the function klass_at_noresolve()'s parameter is an int. This isn't wrong. - PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247802199
Re: RFR: 8311077: Fix -Wconversion warnings in jvmti code [v4]
> Please review change for mostly fixing return types in the constant pool and > metadata to fix -Wconversion warnings in JVMTI code. The order of preference > for changes are: 1. change the types to more distinct types (fields in the > constant pool are u2 because that's their size in the classfile), 2. add > direct int casts if the value has been checked in asserts above, and 3. > checked_cast<> if not verified, and 4. added some pointer_delta_as_ints where > needed. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: David's suggestions. - Changes: - all: https://git.openjdk.org/jdk/pull/14710/files - new: https://git.openjdk.org/jdk/pull/14710/files/42781421..d07a212c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14710&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14710&range=02-03 Stats: 6 lines in 4 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14710.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14710/head:pull/14710 PR: https://git.openjdk.org/jdk/pull/14710
