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

[Kieran Bingham]

Hi Laurent,

On 23/10/2020 05:23, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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.


> 
>> 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 ?
> 
>> + */
>> +#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 :-)
> 
> 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_'...

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).

> 
>> +	/* 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;

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.


> 
>> +	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