RFR: 8328573: Add ASSERT macro and not use 'CardTable::card_shift_in_words' in 'G1BlockOffsetTable::check_index'

[Thomas Schatzl]

On Wed, 20 Mar 2024 07:15:28 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> Hi all,
> 
> This patch adds the ASSERT macro to the declaration and usages of the method `G1BlockOffsetTable::check_index`. And the `CardTable::card_shift_in_words` is changed to `CardTable::card_shift` in order to prepare [JDK-8328508](https://bugs.openjdk.org/browse/JDK-8328508).
> 
> Thanks for taking the time to review.
> 
> Best Regards,
> -- Guoxiong

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1BlockOffsetTable.cpp line 54:

> 52:   assert((index) < (_reserved.byte_size() >> CardTable::card_shift()),
> 53:          "%s - index: " SIZE_FORMAT ", _vs.committed_size: " SIZE_FORMAT,
> 54:          msg, (index), (_reserved.byte_size() >> CardTable::card_shift()));

I do not think these two changes are in any way related to the other assert changes. One may move this change to the mentioned [JDK-8328508](https://bugs.openjdk.org/browse/JDK-8328508).

src/hotspot/share/gc/g1/g1BlockOffsetTable.hpp line 75:

> 73:   inline void set_offset_array(size_t left, size_t right, u_char offset);
> 74: 
> 75: #ifdef ASSERT

I disagree with this change - it is common usage to use the `NOT_DEBUG_RETURN` or similar macros to hide the `#ifdef`s.
The `#ifdef`s only clutter the code for no gain as you can see.

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

PR Review: https://git.openjdk.org/jdk/pull/18391#pullrequestreview-1948240152
PR Review Comment: https://git.openjdk.org/jdk/pull/18391#discussion_r1531684640
PR Review Comment: https://git.openjdk.org/jdk/pull/18391#discussion_r1531683574