[libcamera-devel] [PATCH] libcamera: Remove std::piecewise_construct where not necessary

[Kieran Bingham]

Hi Laurent,

I find the commit message quite terse here, and it doesn't help me in
understanding the meaning behind why the piecewise construct was (or
should) be used, or why it's being removed.

Perhaps that's not quite the purpose of the commit message of course
(it's not supposed to teach us C++), but I feel like this is one point
where a good -easy-to-understand message would be useful. Particularly
if it helps us prevent this mistake in the future.

Now that we've gone through the purpose of the piecewise_constructor,
I've tried to rewrite your commit message below to try to clarify my
understanding.

If you find any of the content more pleasing, or easier to read, feel
free to take it in any form.


On 14/01/2020 20:14, Laurent Pinchart wrote:
> When emplacing an element in a std::map, std::piecewise_construct is
> required to forward the parameters to the constructors of the key and
> mapped type, respectively. However, when the caller already has an
> instance of the mapped type, forwarding it to the mapped type copy
> constructor isn't really required, as the copy constructor would be
> called anyway. Drop std::piecewise_construct in those cases. This does
> not incur any performance cost.
> 


When emplacing a control on to the ControlInfoMap with an ID and a
Range, each of the parameters are objects and are directly attributed to
the construction of the key value pairs of the ControlInfoMap.

The use of std::piecewise_construct permits, for example the range to be
specified using a non-default constructor (such as {0, 100}) directly in
the emplace method, where the parameter values would be forwarded to the
constructor.

In the UVC Video and the VIMC pipeline handlers, this has been overused
as the parameters to the emplace method are objects themselves, and the
emplace can use the copy constructors to create the new ControlInfoMap
entry.

Simplify the code and improve readability by utilising the default
emplace method for the map which takes a single argument for each of the
two elements.

No change is expected in the output code generated by this update.


> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

With or without any update to the commit message, this change is fine.

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


> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 4 +---
>  src/libcamera/pipeline/vimc.cpp     | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 83093676ec73..29afb121aa46 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -365,9 +365,7 @@ int UVCCameraData::init(MediaEntity *entity)
>  			continue;
>  		}
>  
> -		ctrls.emplace(std::piecewise_construct,
> -			      std::forward_as_tuple(id),
> -			      std::forward_as_tuple(range));
> +		ctrls.emplace(id, range);
>  	}
>  
>  	controlInfo_ = std::move(ctrls);
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index c99560a45cfa..b1054d307ea2 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -448,9 +448,7 @@ int VimcCameraData::init(MediaDevice *media)
>  			continue;
>  		}
>  
> -		ctrls.emplace(std::piecewise_construct,
> -			      std::forward_as_tuple(id),
> -			      std::forward_as_tuple(range));
> +		ctrls.emplace(id, range);
>  	}
>  
>  	controlInfo_ = std::move(ctrls);
> 

-- 
Regards
--
Kieran