RFR (S): 8228857: Refactor PlatformMonitor into PlatformMutex and PlatformMonitor

[David Holmes]

Hi Kim,

On 9/08/2019 4:03 pm, Kim Barrett wrote:
>> On Aug 9, 2019, at 1:16 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 9/08/2019 9:17 am, Kim Barrett wrote:
>>> Maybe we should have a helper macro for that, but nobody has proposed adding
>>> one. For C++11 it's
>>>    PlatformMutex(const PlatformMutex&) = delete;
>>>    PlatformMutex& operator=(const PlatformMutex&) = delete;
>>> anywhere in the class (public is best, to get a reference to deleted function
>>> error message rather than possibly an access error message).
>>> This is a "good hygiene" thing; many of our classes "ought" to have this but
>>> don't bother.
>>
>> In my limited experience it seems C++ got it backwards by providing these by default - copying and assignment should be opt-in.
> 
> You won’t get an argument from me on that.  At least the syntax has improved in the newer versions,
> and one can now do things like changing the accessibility without affecting POD-ness and the like.
> 
>>> It imposes virtual function overheads (class vtable, per-object vtable
>>> entries, &etc) on what is intended to be simple, low-overhead classes.
>>
>> Okay ...
>>
>>> Do we care? One option is to ignore the problem; there's certainly precedent
>>
>> Seems like the path of least resistance, and it's relatively easy to check that we don't assign a PlatformMonitor to a PlatformMutex field.
> 
> Being noncopyable takes care of that.

Noncopyable doesn't stop us from doing

PlatformMutex m = new PlatformMonitor();

though, does it?

>>> class PlatformMutexBase ...
>>>    ... same definition as proposed PlatformMutex, except with
>>>        protected constructor and destructor ...
>>> };
>>> class PlatformMutex : public PlatformMutexBase {};
>>> class PlatformMonitor : public PlatformMutexBase {
>>>    ... same definition as proposed PlatformMonitor, adjusting for
>>>        base class name change ...
>>> };
>>> It amounts to one more line of code, the new PlatformMutex class definition.
>>
>> Okay so this supposes that we never declare variables of type PlatformMutexBase (which we shouldn't), […]
> 
> No “shouldn’t” about it; the above scheme actively prevents that.
> 
>    PlatformMutexBase my_thing;
> 
> will compile time error because neither the constructor nor the
> destructor are accessible.
> 
> It is possible to have PlatformMutexBase* values, you just can’t call
> delete on them.  You have to delete or destroy the derived object,
> which is the point.

Okay.

>> but only PlatformMutex and PlatformMonitor, and those two are not assignment compatible and so we avoid the problem.
>>
>> That's not a terrible solution, but if you don't object strongly I'll stick with the current arrangement.
> 
> I don’t think I can object strongly to things that are already pretty endemic in our code.

I believe you can object strongly if you think we should be setting new 
standards of code cleanliness in new code. Though in that case we should 
get them codified in the style guide ASAP.

Thanks,
David