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

[Laurent Pinchart]

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.

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

> +	/* copy constructor. */         \

I think you can drop the comments, it's quite explicit already.

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

Laurent Pinchart