[libcamera-devel] [PATCH 1/5] libcamera: Provide class.h

[Kieran Bingham]

Hi Laurent,

On 23/10/2020 21:11, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Oct 23, 2020 at 09:16:55AM +0100, Kieran Bingham wrote:
>> On 23/10/2020 05:23, Laurent Pinchart wrote:
>>> On Thu, Oct 22, 2020 at 02:56:01PM +0100, Kieran Bingham wrote:
>>>> Provide a public header to define helpers for class definitions.
>>>> This initially implements macros to clearly define the deletion of
>>>> copy/move/assignment constructors/operators for classes to restrict
>>>> their capabilities explicitly.
>>>
>>> Thanks for picking this up after my unsuccessful experimented with a
>>> non-copyable base class. There's a few bikesheeding comments beow, but
>>> idea looks good.
>>>
>>> Have you considered including this in a more generic header, such as
>>> utils.h (or global.h, or <bikeshedding>.h) ? I don't foresee lots of new
>>> features to be added to class.h, while I think we'll have more
>>> miscellaneous macros and functions moving forward. It would be nice to
>>> avoid creating micro-headers.
>>
>> That's why I chose class.h ...
>>
>> I thought this would cover more than just these macros.
>>
>> Throwing your argument back at you, You've recently posted a patch which
>> adds 'extensible.h' ... and both these, and your extensible concepts are
>> attributed to 'class' helpers, (yet these could not go in to
>> extensible.h) ...
>>
>> So ... Would you prefer to put your extensible types in class.h, or
>> global.h?
>>
>> We have utils.h of course, but that's in internal/ and this needs to be
>> in public header space.
>>
>> Of course we could also have a public utils.h ... but I'm a bit opposed
>> to having lots of duplicated header names in both public and private
>> space, as it gets confusing as to which one is being used.
> 
> The move of the private headers to an internal/ directory was meant to
> allow public and private headers with the same name, but it of course
> doesn't mean we have to (ab)use that feature.
> 
> Merging these macros and the Extensible class in a single header makes
> sense to me. That broadens the scope of class.h a bit so we could go
> with that. I still feel we'll have a need for miscellaneous public
> "things" in the future which wouldn't be a good match for class.h, but
> we could address that problem at that time.

Now that you have merged the Extensible.h header, do you have any
preference for where these helpers would live?


>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> ---
>>>>  include/libcamera/class.h     | 35 +++++++++++++++++++++++++++++++++++
>>>>  include/libcamera/meson.build |  1 +
>>>>  2 files changed, 36 insertions(+)
>>>>  create mode 100644 include/libcamera/class.h
>>>>
>>>> diff --git a/include/libcamera/class.h b/include/libcamera/class.h
>>>> new file mode 100644
>>>> index 000000000000..adcfa8860957
>>>> --- /dev/null
>>>> +++ b/include/libcamera/class.h
>>>> @@ -0,0 +1,35 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2020, Google Inc.
>>>> + *
>>>> + * class.h - class helpers
>>>> + */
>>>> +#ifndef __LIBCAMERA_CLASS_H__
>>>> +#define __LIBCAMERA_CLASS_H__
>>>> +
>>>> +/**
>>>> + * \brief Delete the copy constructor and assignment operator.
>>>
>>> Missing \param ?
>>>

How about:

+ * \param klass The identifier of the class to modify


>>>> + */
>>>> +#define DELETE_COPY_AND_ASSIGN(klass)   \
>>>
>>> Macros are unfortunately not part of namespaces, so to avoid a risk of
>>> collision, I'd use a LIBCAMERA_ prefix. And to avoid the name getting
>>> too long, I think you can drop the _AND_ASSIGN suffix, as the macro is
>>> disabling copying as a whole. Otherwise it should be
>>> DELETE_COPY_CONSTRUCT_AND_ASSIGN :-)

LIBCAMERA_ feels a bit extraneous to me ... but I can add it.
(We could also wrap it in a #ifndef DELETE_COPY_xxxxx)

DELETE_COPY_CONSTRUCT_AND_ASSIGN is really quite long...
and
LIBCAMERA_DELETE_COPY_CONSTRUCT_AND_ASSIGN
even more so.

>>>
>>> Another bikeshedding topic, I would have gone for DISABLE instead of
>>> DELETE to emphasize the purpose of the macro instead of how it's
>>> implemented (not that libcamera has to care about this, but in older C++
>>> versions we would have needed to declare the functions as private as
>>> there was no = delete).
>>
>> That's why I chose delete, to express explicitly that this is 'deleting'
>> the functions.
>>
>> The similar 'google' macros are called 'DISALLOW_'...
> 
> And one more data point,
> https://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY :-)
> 
> I prefer expressing the purpose in the name rather than the
> implementation details. That arguments makes more sense for Qt that has
> to support multiple C++ versions, including older versions where =
> delete wasn't available.

I still prefer delete ...

These macros are providing syntactic sugar to make sure 'what' is being
deleted is explicit (in human readable terms).

And I don't believe we'll be supporting a C++ version which can't handle
= delete ?


> If you prefer DELETE over DISABLE I won't fight hard for it (we know we
> want to avoid DISALLOW as that's what Google deprecates, but DISABLE or
> DELETE are not deprecated, right ? ;-)). I feel a bit stronger about
> writing 'COPY' instead of 'COPY_AND_ASSIGN', as the copy concept is
> really the combination of a constructor and an assignment operator (same
> for move).

Tell me what colour you'd like this and I'll buy the paint.

Would you like to just copy QT and go with

LIBCAMERA_DISABLE_COPY
LIBCAMERA_DISABLE_MOVE?


>> In fact, somewhat frustratingly, the chromium style guides deprecated
>> the equivalent macros:
>>
>> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md
>>
>> And instead prefer to code them directly. But I disagree, as we've seen
>> this can lead to errors, both in not deleting both the constructor and
>> assignment operator, or in getting the names wrong, and quite honestly I
>> find them really hard to interpret, just from looking at them. (More below).
> 
> I agree with you, I think the macros make sense. We wouldn't be having
> this conversation if just deleting the correct functions wasn't
> error-prone.
> 
>>>
>>>> +	/* copy constructor. */         \
>>>
>>> I think you can drop the comments, it's quite explicit already.
>>
>> The reason I've put these here, is because I find it really hard to look
>> at these and say "Oh - that's the copy constructor", (as opposed to the
>> move constructor), and "That's the copy assignment operator", as opposed
>> to the move assignment operator.
>>
>> Lets consider it like the C++ equivalent of English Homophones. Things
>> that look/read/sound similar but have a different meaning.
>>
>> i.e. : If I hadn't looked at this in more than one day, I would have to
>> look up which of these does which :
>> 	klass(klass &&) = delete;
>> 	klass(const klass &) = delete;


Wow - coming back at this, and I really have to look up which version of
each does what. Which only solidifies in my head that these would be
much better with names.


>>
>> and equally for the operators:
>> 	klass &operator=(const klass &) = delete
>> 	klass &operator=(klass &&) = delete
>>
>> And in code - I don't want the meaning of the code to be easy to
>> mis-interpret.
> 
> Maybe it's a matter of getting used to it :-) My comment was more about

Perhaps someone who writes move/copy constructors everyday would easily
remember the declarations and it would become as obvious as reading a
for-loop... but they're a low churn code type, and often copied as a
template anyway.


> the fact that telling the constructor and assignment operator apart is
> fairly straightforward (at least more than telling the copy and move
> versions apart at a quick glance), and which pair you delete is part of
> the macro name. That's why I didn't consider the comments to add much,
> but if you think they help, I don't mind (we usually avoid comments in
> headers though, but there are exceptions where it makes sense).
> 
>>>> +	klass(const klass &) = delete;  \
>>>> +	/* copy assignment operator. */ \
>>>> +	klass &operator=(const klass &) = delete
>>>> +
>>>> +/**
>>>> + * \brief Delete the move construtor and assignment operator.
>>>> + */
>>>> +#define DELETE_MOVE_AND_ASSIGN(klass)   \
>>>> +	/* move constructor. */         \
>>>> +	klass(klass &&) = delete;       \
>>>> +	/* move assignment operator. */ \
>>>> +	klass &operator=(klass &&) = delete
>>>> +
>>>> +/**
>>>> + * \brief Delete all copy and move constructors, and assignment operators.
>>>> + */
>>>> +#define DELETE_COPY_MOVE_AND_ASSIGN(klass) \
>>>> +	DELETE_COPY_AND_ASSIGN(klass);     \
>>>> +	DELETE_MOVE_AND_ASSIGN(klass)
>>>> +
>>>> +#endif /* __LIBCAMERA_CLASS_H__ */
>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>>> index 3d5fc70134ad..b3363a6f735b 100644
>>>> --- a/include/libcamera/meson.build
>>>> +++ b/include/libcamera/meson.build
>>>> @@ -5,6 +5,7 @@ libcamera_public_headers = files([
>>>>      'buffer.h',
>>>>      'camera.h',
>>>>      'camera_manager.h',
>>>> +    'class.h',
>>>>      'controls.h',
>>>>      'event_dispatcher.h',
>>>>      'event_notifier.h',
> 

-- 
Regards
--
Kieran