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.
-------------
Commit messages:
- Merge remote-tracking branch 'valhalla/lworld' into
lworld_8374016_ResolvedFieldEntry
- 8374016: [lworld] Too late checked_cast in ResolvedFieldEntry::set_flags
- 8374007: [lworld] Remove dead code in Symbol
Changes: https://git.openjdk.org/valhalla/pull/1844/files
Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1844&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8374016
Stats: 28 lines in 2 files changed: 8 ins; 2 del; 18 mod
Patch: https://git.openjdk.org/valhalla/pull/1844.diff
Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1844/head:pull/1844
PR: https://git.openjdk.org/valhalla/pull/1844
