On Fri, 19 Dec 2025 12:32:03 GMT, Stefan Karlsson <[email protected]> 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. Good point about the checked_cast. Style changes are good if they make the code more explicit to the reader. Thank you for these fixes. ------------- Marked as reviewed by fparain (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1844#pullrequestreview-3599070561
