On Tue, 17 Dec 2024 06:57:59 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> CML could perhaps be used has-a (though that might be messy), but should not
>> be used is-a. One of the basic rules for base classes is to have either a
>> public virtual destructor (don't do that here) or a non-public non-virtual
>> destructor, to prevent accidental slicing. MutexLockerImpl has that - it was
>> designed to be a base class. CML doesn't - it was not designed to be a base
>> class. (There is a common (though not universal) opinion that classes should
>> either be base or concrete classes, not both.) I know we violate this rule
>> all
>> over the place; that doesn't mean we should do so here.
>>
>> But really, there's no benefit to using CML here. Deriving from
>> MutexLockerImpl is about the same number of characters, and seems clearer to
>> me because it's more direct.
>>
>>
>> class NmtVirtualMemoryLocker : public MutexLockerImpl {
>> public:
>> NmtVirtualMemoryLocker()
>> : MutexLockerImpl(_done_bootstrap ? NmtVirtualMemory_lock : nullptr,
>> Mutex::_no_safepoint_check_flag)
>> {}
>> }
>>
>>
>> Related, I just noticed that MutexLockerImpl is copyable, but shouldn't be.
>> (I remember a discussion a long time ago about whether StackObj should be
>> noncopyable, which foundered on a combination of some significant number of
>> existing uses that violated that restriction, along with disagreement about
>> the meaning of deriving from StackObj. So we're not getting noncopyable from
>> there.)
>
> `MutexLockerImpl` was not intended for external subclassing, but as the
> internal base class for `MutexLocker`, and `ConditionalMutexLocker`. CML was
> provided for exactly this kind of situation: conditionally locking the mutex.
Ok I see. I will stop using `ConditionalMutexLocker` as a base class. Instead,
I'll just get rid of `NmtVirtualMemoryLocker` and use `ConditionalMutexLocker`
directly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1888546778
