[lworld] RFR: 8374016: [lworld] Too late checked_cast in ResolvedFieldEntry::set_flags

[Matias Saavedra Silva]

On Fri, 19 Dec 2025 12:32:03 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> In the lworld branch the `new_flags` type has been changed to u1 instead of int. We now perform a bunch of bit operations, which results in an int, and then we narrow that down into a u1. After the u1 has been created we check that this doesn't overflow a u1 with the checked_cast<u1> check. That's too late.
> 
> Mainline code looks like this:
> 
> int new_flags = ...
> _flags = checked_cast<u1>(new_flags);
> 
> 
> and Valhalla code looks like this:
> 
> u1 new_flags = ...
> _flags = checked_cast<u1>(new_flags);
> 
> 
> This voids the benefit of having a checked_cast here. I propose that we revert back to using `int new_flags` and keep the checked_cast<u1>.
> 
> While reading this code there were a few things that made the code harder to follow and I've tweaked it.
> 
> 1) The `bool` `is_volatile_shift` is special-treated and instead of shifting it is directly casted to int. That hard codes a knowledge/assumption that the flag is always going to be 1 (and maybe also what value bools have).
> 
> That made me have to look at the actual values of these and at one point I thought that is_volatile_shift was `1` and not `0` and that the above was a bug. The main reason I thought that was this odd order here:
> 
>    u1 new_flags = ((is_final_flag ? 1 : 0) << is_final_shift) | static_cast<int>(is_volatile_flag) |
>       ((is_flat_flag ? 1 : 0) << is_flat_shift) |
>       ((is_null_free_inline_type_flag ? 1 : 0) << is_null_free_inline_type_shift) |
>       ((has_null_marker_flag ? 1 : 0) << has_null_marker_shift);
> 
> Here the order of usage is
> 
> is_final_flag
> is_volatile_flag
> is_flat_flag
> is_null_free_inline_type_flag
> has_null_marker_flag
> 
> 
> but the enum order has final and volatile swapped:
> 
> is_volatile_shift
> is_final_shift
> is_flat_shift
> is_null_free_inline_type_shift
> has_null_marker_shift
> 
> 
> I'd prefer if we stayed consistent with the order, to lower the risk that some bugs sneak in here. So, I've replaced the cast with a shift (just like the other values are handled) and I  ordered functions, parameters, and operations in the same order as the enums.
> 
> 2) Tweaked some alignments to make these kind of issues clearer.
> 
> Please tell me if I went to far with the style changes.

Change looks good and makes the code more clear, thanks!

-------------

Marked as reviewed by matsaave (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1844#pullrequestreview-3599398946