RFR: 8245025: MoveAndUpdateClosure::do_addr calls function with side-effects in an assert

[Thomas Schatzl]

On Tue, 9 Mar 2021 20:08:23 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> Make the assertion code side-effect free.

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 831:

> 829:     guarantee(trueInDebug, "Only in debug build");
> 830:     auto bitmap = PSParallelCompact::mark_bitmap();
> 831:     auto live = region_ptr->partial_obj_size()

Please do not use `auto` as type for trivial types. Using `auto` makes it very hard for reviewers to understand the code sometimes, requiring him/her to look around what could be meant (as at least I had to do after "something" is added to `result` later). The (intended) type of a variable is part of the documentation for readers (e.g. for the reviewer). I.e. requiring the reader to infer the context (and applying the current language's rules for determining the value) before being able to read it.

I understand that it is easier for the developer to type just `auto` everywhere, but there will be many more people looking at the code than writing it during the lifetime of a piece of code.

Other than that I'm okay with the code duplication in the last few lines of each of the branches because it's probably not worth trying to reshape it a bit - although I'd have preferred to have `live` contain just the result of `bitmap->live_words_in_range(..)` in both cases, and determine the result in the next expression as sum of `block_offset` and `live`.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 832:

> 830:     auto bitmap = PSParallelCompact::mark_bitmap();
> 831:     auto live = region_ptr->partial_obj_size()
> 832:               + bitmap->live_words_in_range(cm, region_align_down(addr), oop(addr));

The operator (`+`) should be on the preceeding line as shown in the same file.

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

PR: https://git.openjdk.java.net/jdk/pull/2898