Improve V4L2CameraFile constructor implementation
diff mbox series

Message ID e6fc4a59-7f21-4451-b80b-4ba8ff0e6532@web.de
State New
Headers show
Series
  • Improve V4L2CameraFile constructor implementation
Related show

Commit Message

Markus Elfring Feb. 5, 2026, 7:25 p.m. UTC
Hello,

I suggest to take another look also at implementation details according to
the commit “v4l2: v4l2_camera_file: Store file description” from 2021-12-28.
https://gitlab.freedesktop.org/camera/libcamera/-/commit/07d5fff29ce96f3456cf74d740b56a46c5cba507#b2c8de5ff31c2c413e72dfb673beb004acd9fbbe_21_27

How do you think about to integrate a small source code adjustment
like the following?





Regards,
Markus

Comments

Paul Elder Feb. 6, 2026, 4:01 p.m. UTC | #1
Hi Markus,

Quoting Markus Elfring (2026-02-06 04:25:46)
> Hello,
> 
> I suggest to take another look also at implementation details according to
> the commit “v4l2: v4l2_camera_file: Store file description” from 2021-12-28.
> https://gitlab.freedesktop.org/camera/libcamera/-/commit/07d5fff29ce96f3456cf74d740b56a46c5cba507#b2c8de5ff31c2c413e72dfb673beb004acd9fbbe_21_27
> 
> How do you think about to integrate a small source code adjustment
> like the following?

Besides the fact that this is missing a Signed-off-by tag [0], I don't
understand what this code change aims to achieve. What benefit does the
append() have over concatenation with +?

[0] https://libcamera.org/contributing.html#submitting-patches


Paul

> 
> 
> diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp
> index d8fe854b..aa1299ad 100644
> --- a/src/v4l2/v4l2_camera_file.cpp
> +++ b/src/v4l2/v4l2_camera_file.cpp
> @@ -28,17 +28,22 @@ V4L2CameraFile::V4L2CameraFile(int dirfd, const char *path, int efd,
>                 if (dirfd == AT_FDCWD) {
>                         char *cwd = getcwd(nullptr, 0);
>                         if (cwd) {
> -                               description_ = std::string(cwd) + "/";
> +                               description_ = std::string(cwd);
> +                               description_.push_back('/');
>                                 free(cwd);
>                         } else {
>                                 description_ = std::string("(unreachable)/");
>                         }
>                 } else {
> -                       description_ = "(dirfd:" + std::to_string(dirfd) + ")/";
> +                       description_ = std::string("(dirfd:");
> +                       description_.append(std::to_string(dirfd)).append(")/");
>                 }
>         }
>  
> -       description_ += std::string(path) + " (fd:" + std::to_string(efd) + ")";
> +       description_.append(std::string(path))
> +                   .append(" (fd:")
> +                   .append(std::to_string(efd))
> +                   .push_back(')');
>  }
>  
>  V4L2CameraFile::~V4L2CameraFile()
> 
> 
> 
> Regards,
> Markus
Markus Elfring Feb. 6, 2026, 5:12 p.m. UTC | #2
> Besides the fact that this is missing a Signed-off-by tag [0],

Thanks for such a reminder.


> I don't understand what this code change aims to achieve. What benefit does the
> append() have over concatenation with +?

Various information sources provide guidance for a recurring development topic
like “Efficient string concatenation in C++”.
Is there a need to repeat known advices?

Regards,
Markus

Patch
diff mbox series

diff --git a/src/v4l2/v4l2_camera_file.cpp b/src/v4l2/v4l2_camera_file.cpp
index d8fe854b..aa1299ad 100644
--- a/src/v4l2/v4l2_camera_file.cpp
+++ b/src/v4l2/v4l2_camera_file.cpp
@@ -28,17 +28,22 @@  V4L2CameraFile::V4L2CameraFile(int dirfd, const char *path, int efd,
 		if (dirfd == AT_FDCWD) {
 			char *cwd = getcwd(nullptr, 0);
 			if (cwd) {
-				description_ = std::string(cwd) + "/";
+				description_ = std::string(cwd);
+				description_.push_back('/');
 				free(cwd);
 			} else {
 				description_ = std::string("(unreachable)/");
 			}
 		} else {
-			description_ = "(dirfd:" + std::to_string(dirfd) + ")/";
+			description_ = std::string("(dirfd:");
+			description_.append(std::to_string(dirfd)).append(")/");
 		}
 	}
 
-	description_ += std::string(path) + " (fd:" + std::to_string(efd) + ")";
+	description_.append(std::string(path))
+		    .append(" (fd:")
+		    .append(std::to_string(efd))
+		    .push_back(')');
 }
 
 V4L2CameraFile::~V4L2CameraFile()