RFR: 8342560: GenShen: Fix confusing method name

Y. Srinivas Ramakrishna ysr at openjdk.org
Thu Oct 17 21:49:24 UTC 2024


On Thu, 17 Oct 2024 20:56:59 GMT, William Kemper <wkemper at openjdk.org> wrote:

> Rename `is_old` to make it more clear that this about pointers that are outside the scope of a young collection.

A couple of comments. Will re-review again after your response. Thanks!

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 606:

> 604:   inline bool is_in_young(const void* p) const;
> 605:   inline bool is_in_old(const void* p) const;
> 606:   inline bool is_not_in_active_young_collection(oop obj) const;

Needs a single line documentation comment, given its not very natural/intuitive semantics.

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 394:

> 392: }
> 393: 
> 394: inline bool ShenandoahHeap::is_not_in_active_young_collection(oop obj) const {

This sounds to me more like: `is_in_old_when_active_young()`.

The "not" is the name makes the negative of its semantics suspect. Think of the sense of what one might believe if one identified the negation with `is_in_active_young_collection()` which I'd take to be `active_generation()->is_young() && is_in_young(obj)`, rather than the actual negation of the condition which would be: `!active_generation()->is_young() || !is_in_old(obj)`.

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

PR Review: https://git.openjdk.org/shenandoah/pull/517#pullrequestreview-2376364492
PR Review Comment: https://git.openjdk.org/shenandoah/pull/517#discussion_r1805481523
PR Review Comment: https://git.openjdk.org/shenandoah/pull/517#discussion_r1805480760


More information about the shenandoah-dev mailing list