RFR: 8348349: Refactor CDSConfig::is_dumping_heap() [v2]

Ioi Lam iklam at openjdk.org
Mon Jan 27 06:02:54 UTC 2025


On Thu, 23 Jan 2025 22:14:06 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @matias9927 comment
>
> src/hotspot/share/cds/cdsConfig.cpp line 572:
> 
>> 570: bool CDSConfig::is_dumping_heap() {
>> 571:   // heap dump is not supported in dynamic dump
>> 572:   if (!is_dumping_static_archive()) {
> 
> I think you can compress these three cases into one if-statement

Fixed.

> src/hotspot/share/cds/cdsConfig.hpp line 49:
> 
>> 47: 
>> 48:   static bool  _old_cds_flags_used;
>> 49:   static bool  _disable_heap_dumping;
> 
> Following with the previous implementation, I'd prefer `can_dump_heap` to avoid the double negative.

`_disable_heap_dumping` is for unconditionally disabling heap dumping. The opposite does not mean we can dump the heap -- heap dumping can also be disabled if ZGC is used. So `can_dump_heap` will be misleading.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23249#discussion_r1930017837
PR Review Comment: https://git.openjdk.org/jdk/pull/23249#discussion_r1930017826


More information about the hotspot-dev mailing list