On Thu, 9 Apr 2026 12:32:12 GMT, Paul Hübner <[email protected]> wrote:
>> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update markWord.hpp > > src/hotspot/share/oops/markWord.hpp line 142: > >> 140: static const int hash_shift = >> valhalla_reserved_shift + valhalla_reserved_bits; >> 141: >> 142: #define mask_in_place(bits, shift) (right_n_bits(bits) << (shift)) > > Nit: I skimmed the HotSpot style guide and couldn't find anything regarding > case, but I was under the impression we used all uppercase for runtime > macros. For example, `#define DISABLE_FLAG_AND_WARN_IF_NOT_DEFAULT(flag)`. You are probably right, but I really dislike seeing screamy macros in my code. I would rather just get rid of these macros and checks just to not have to pollute this code with upper-case macros. I had one version of this that used template functions, but I couldn't get it to work confined to the markWord class. One alternative would be to put those functions high up in the file, but I prefer to keep the markWord class the first entity in this file. Another alternative would be to put it in globalDefinitions.hpp, but `bit_in_place` is somewhat obscure because of the static assert. Let me think a little bit about this. > src/hotspot/share/oops/markWord.hpp line 156: > >> 154: static const uintptr_t null_free_array_bit_in_place = >> bit_in_place(null_free_array_bits, null_free_array_shift); >> 155: static const uintptr_t flat_array_bit_in_place = >> bit_in_place(flat_array_bits, flat_array_shift); >> 156: static const uintptr_t valhalla_reserved_bit_in_place = >> bit_in_place(valhalla_reserved_bits, valhalla_reserved_shift); > > Nit: alignment. Could you clarify what alignment you want me to fix? There's a pre-existing alignment issue because these names are so long. Fixing that would require re-aligning all constants in this file. I can do that, but I think it would be nice to do so as a follow-up. But maybe there was some other alignment issue you were thinking about. I saw a double space in the vicinity, so I removed that one. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3057996989 PR Review Comment: https://git.openjdk.org/valhalla/pull/2310#discussion_r3058022612
