[libcamera-devel,07/30] cam: options: Disable copy for parsers

12820 diff mbox series

Copying the OptionsParser class would result in the optionsMap_ entries
pointing to Option entries of the original instance. As there's no use
case for copying the class, disable copying.

Disable copying of KeyValueParser as well for consistency as there's no
use case either.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/options.h | 6 ++++++
 1 file changed, 6 insertions(+)

On 07/07/2021 03:19, Laurent Pinchart wrote:
> Copying the OptionsParser class would result in the optionsMap_ entries
> pointing to Option entries of the original instance. As there's no use
> case for copying the class, disable copying.
> 
> Disable copying of KeyValueParser as well for consistency as there's no
> use case either.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/cam/options.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 01a5d36a63fb..4418e201bf1f 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -68,6 +68,9 @@ public:
>  	virtual Options parse(const char *arguments);
>  
>  private:

I will always think these implementations deserve an easily human
readable comment to make sure the intention is clear to the casual reader.

	/* Disable copying of the class. */
> +	KeyValueParser(const KeyValueParser &) = delete;
> +	KeyValueParser &operator=(const KeyValueParser &) = delete;
> +
>  	friend class OptionsParser;
>  	void usage(int indent);
>  
> @@ -95,6 +98,9 @@ public:
>  	void usage();
>  
>  private:
> +	OptionsParser(const OptionsParser &) = delete;
> +	OptionsParser &operator=(const OptionsParser &) = delete;
> +
>  	void usageOptions(const std::list<Option> &options, unsigned int indent);
>  
>  	std::list<Option> options_;
>

Hi Kieran,

On Mon, Jul 12, 2021 at 02:39:09PM +0100, Kieran Bingham wrote:
> On 07/07/2021 03:19, Laurent Pinchart wrote:
> > Copying the OptionsParser class would result in the optionsMap_ entries
> > pointing to Option entries of the original instance. As there's no use
> > case for copying the class, disable copying.
> > 
> > Disable copying of KeyValueParser as well for consistency as there's no
> > use case either.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/cam/options.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/cam/options.h b/src/cam/options.h
> > index 01a5d36a63fb..4418e201bf1f 100644
> > --- a/src/cam/options.h
> > +++ b/src/cam/options.h
> > @@ -68,6 +68,9 @@ public:
> >  	virtual Options parse(const char *arguments);
> >  
> >  private:
> 
> I will always think these implementations deserve an easily human
> readable comment to make sure the intention is clear to the casual reader.
> 
> 	/* Disable copying of the class. */

I suppose it depends on how familiar the reader is with the need to
disable copying :-) If you think it's better, I can add that comment.

> > +	KeyValueParser(const KeyValueParser &) = delete;
> > +	KeyValueParser &operator=(const KeyValueParser &) = delete;
> > +
> >  	friend class OptionsParser;
> >  	void usage(int indent);
> >  
> > @@ -95,6 +98,9 @@ public:
> >  	void usage();
> >  
> >  private:
> > +	OptionsParser(const OptionsParser &) = delete;
> > +	OptionsParser &operator=(const OptionsParser &) = delete;
> > +
> >  	void usageOptions(const std::list<Option> &options, unsigned int indent);
> >  
> >  	std::list<Option> options_;