[PATCH v1] treewide: Avoid some copies in range-based for loops

[Laurent Pinchart]

Hi Barnabás,

(CC'ing Harvey)

Thank you for the patch.

On Tue, Nov 26, 2024 at 06:03:10PM +0000, Barnabás Pőcze wrote:
> Most of these have been found by the `performance-for-range-copy`
> check of `clang-tidy`.
> 
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/libcamera/camera_manager.cpp                         | 2 +-
>  src/libcamera/converter.cpp                              | 5 +++--
>  src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +-
>  src/libcamera/pipeline_handler.cpp                       | 2 +-
>  test/gstreamer/gstreamer_device_provider_test.cpp        | 9 ++-------
>  5 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index c7cc5adb..87e6717e 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -388,7 +388,7 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &id)
>  
>  	MutexLocker locker(d->mutex_);
>  
> -	for (std::shared_ptr<Camera> camera : d->cameras_) {
> +	for (const std::shared_ptr<Camera> &camera : d->cameras_) {
>  		if (camera->id() == id)
>  			return camera;
>  	}
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 945f2527..3a3f8434 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -325,8 +325,9 @@ std::vector<std::string> ConverterFactoryBase::names()
>  
>  	for (ConverterFactoryBase *factory : factories) {
>  		list.push_back(factory->name_);
> -		for (auto alias : factory->compatibles())
> -			list.push_back(alias);
> +
> +		const auto &compatibles = factory->compatibles();
> +		list.insert(list.end(), compatibles.begin(), compatibles.end());
>  	}
>  
>  	return list;
> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> index 2baef588..277efbb0 100644
> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp
> @@ -39,7 +39,7 @@ ImageFrameGenerator::create(ImageFrames &imageFrames)
>  	 * For each file in the directory, load the image,
>  	 * convert it to NV12, and store the pointer.
>  	 */
> -	for (std::filesystem::path path : imageFrames.files) {
> +	for (const auto &path : imageFrames.files) {

Unrelated to this patch, just because I notice this now in the virtual
pipeline handler that was merged recently. A while ago, we merged

commit 6a2f971035c2df711b10200f9c8c011d9a420e58
Author: Nicholas Roth <nicholas at rothemail.net>
Date:   Thu Oct 27 22:17:21 2022 -0500

    android: remove references to std::filesystem

    Android 11's toolchain does not support std::filesystem, but
    camera_hal_config.cpp currently uses it. Remove references to
    std::filesystem in order to support Android <= 11.

We should drop usage of std::filesystem in
src/libcamera/pipeline/virtual/. I've been thinking of either extending
the libcamera base File class, or implementing new classes, with APIs
influenced by https://doc.qt.io/qt-6/qfileinfo.html and
https://doc.qt.io/qt-6/qdir.html. This would also replace existing use
of stat() and fstat() through the code base.

>  		File file(path);
>  		if (!file.open(File::OpenModeFlag::ReadOnly)) {
>  			LOG(Virtual, Error) << "Failed to open image file " << file.fileName()
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 991b06f2..caa5c20e 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -74,7 +74,7 @@ PipelineHandler::PipelineHandler(CameraManager *manager)
>  
>  PipelineHandler::~PipelineHandler()
>  {
> -	for (std::shared_ptr<MediaDevice> media : mediaDevices_)
> +	for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
>  		media->release();
>  }
>  
> diff --git a/test/gstreamer/gstreamer_device_provider_test.cpp b/test/gstreamer/gstreamer_device_provider_test.cpp
> index 8b8e7cba..4b087fcb 100644
> --- a/test/gstreamer/gstreamer_device_provider_test.cpp
> +++ b/test/gstreamer/gstreamer_device_provider_test.cpp
> @@ -52,17 +52,12 @@ protected:
>  		for (l = devices; l != NULL; l = g_list_next(l)) {
>  			GstDevice *device = GST_DEVICE(l->data);
>  			g_autofree gchar *gst_name;
> -			bool matched = false;
>  
>  			g_autoptr(GstElement) element = gst_device_create_element(device, NULL);
>  			g_object_get(element, "camera-name", &gst_name, NULL);
>  
> -			for (auto name : cameraNames) {
> -				if (strcmp(name.c_str(), gst_name) == 0) {
> -					matched = true;
> -					break;
> -				}
> -			}
> +			bool matched =
> +				std::find(cameraNames.begin(), cameraNames.end(), gst_name) != cameraNames.end();
>  
>  			if (!matched)
>  				return TestFail;

Maybe you could now drop the matched variable and write

			if (std::find(cameraNames.begin(), cameraNames.end(), gst_name) !=
			    cameraNames.end())
				return TestFail;

?

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

This makes me wish C++ had std::contains(). I suppose C++23's
std::ranges:contains() will do the job in several years. There's also

			if (std::any_of(cameraNames.begin(), cameraNames.end(),
				        [&gst_name](const std::string &name) { return name == gst_name; })
				return TestFail;

which I don't think would be much of an improvement :-)

-- 
Regards,

Laurent Pinchart