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

[Laurent Pinchart]

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.

> >> 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_'...

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.

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

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

Laurent Pinchart