[libcamera-devel] cam: Use structured bindings in range-based for loops
diff mbox series

Message ID 20220220005257.30480-1-galof.nejc@gmail.com
State Accepted
Commit 7a118dbdb881c38ad13de12efb023c6b78d2a57e
Headers show
Series
  • [libcamera-devel] cam: Use structured bindings in range-based for loops
Related show

Commit Message

Nejc Galof Feb. 20, 2022, 12:52 a.m. UTC 
Use structured bindings range-based for loops for better readability.
---
 src/cam/camera_session.cpp | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

Comments

Laurent Pinchart Feb. 21, 2022, 12:49 p.m. UTC | #1 
Hi Nejc,

Thank you for the patch.

On Sun, Feb 20, 2022 at 01:52:57AM +0100, Nejc Galof wrote:
> Use structured bindings range-based for loops for better readability.

It looks nicer indeed !

> ---
>  src/cam/camera_session.cpp | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index 1bf460fa..0428b538 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -120,10 +120,7 @@ CameraSession::~CameraSession()
>  
>  void CameraSession::listControls() const
>  {
> -	for (const auto &ctrl : camera_->controls()) {
> -		const ControlId *id = ctrl.first;
> -		const ControlInfo &info = ctrl.second;
> -
> +	for (const auto &[id, info] : camera_->controls()) {
>  		std::cout << "Control: " << id->name() << ": "
>  			  << info.toString() << std::endl;
>  	}

You can now drop the curly braces.

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

There's no need to submit a v2, I'll make this small change when
applying the patch.

> @@ -131,9 +128,8 @@ void CameraSession::listControls() const
>  
>  void CameraSession::listProperties() const
>  {
> -	for (const auto &prop : camera_->properties()) {
> -		const ControlId *id = properties::properties.at(prop.first);
> -		const ControlValue &value = prop.second;
> +	for (const auto &[key, value] : camera_->properties()) {
> +		const ControlId *id = properties::properties.at(key);
>  
>  		std::cout << "Property: " << id->name() << " = "
>  			  << value.toString() << std::endl;
> @@ -374,10 +370,7 @@ void CameraSession::processRequest(Request *request)
>  	     << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
>  	     << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
>  
> -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> -		const Stream *stream = it->first;
> -		FrameBuffer *buffer = it->second;
> -
> +	for (const auto &[stream, buffer] : buffers) {
>  		const FrameMetadata &metadata = buffer->metadata();
>  
>  		info << " " << streamNames_[stream]
> @@ -401,10 +394,10 @@ void CameraSession::processRequest(Request *request)
>  
>  	if (printMetadata_) {
>  		const ControlList &requestMetadata = request->metadata();
> -		for (const auto &ctrl : requestMetadata) {
> -			const ControlId *id = controls::controls.at(ctrl.first);
> +		for (const auto &[key, value] : requestMetadata) {
> +			const ControlId *id = controls::controls.at(key);
>  			std::cout << "\t" << id->name() << " = "
> -				  << ctrl.second.toString() << std::endl;
> +				  << value.toString() << std::endl;
>  		}
>  	}
>
Laurent Pinchart Feb. 21, 2022, 12:57 p.m. UTC | #2 
Hi Nejc,

On Mon, Feb 21, 2022 at 02:49:46PM +0200, Laurent Pinchart wrote:
> On Sun, Feb 20, 2022 at 01:52:57AM +0100, Nejc Galof wrote:
> > Use structured bindings range-based for loops for better readability.
> 
> It looks nicer indeed !

I forgot to mention that the patch is missing your Signed-off-by line.
Please see https://libcamera.org/contributing.html#submitting-patches.
You can use the -s option to git commit to add it. For this patch, you
can just reply to this e-mail with the SoB line and I'll add it locally.

> > ---
> >  src/cam/camera_session.cpp | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index 1bf460fa..0428b538 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -120,10 +120,7 @@ CameraSession::~CameraSession()
> >  
> >  void CameraSession::listControls() const
> >  {
> > -	for (const auto &ctrl : camera_->controls()) {
> > -		const ControlId *id = ctrl.first;
> > -		const ControlInfo &info = ctrl.second;
> > -
> > +	for (const auto &[id, info] : camera_->controls()) {
> >  		std::cout << "Control: " << id->name() << ": "
> >  			  << info.toString() << std::endl;
> >  	}
> 
> You can now drop the curly braces.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> There's no need to submit a v2, I'll make this small change when
> applying the patch.
> 
> > @@ -131,9 +128,8 @@ void CameraSession::listControls() const
> >  
> >  void CameraSession::listProperties() const
> >  {
> > -	for (const auto &prop : camera_->properties()) {
> > -		const ControlId *id = properties::properties.at(prop.first);
> > -		const ControlValue &value = prop.second;
> > +	for (const auto &[key, value] : camera_->properties()) {
> > +		const ControlId *id = properties::properties.at(key);
> >  
> >  		std::cout << "Property: " << id->name() << " = "
> >  			  << value.toString() << std::endl;
> > @@ -374,10 +370,7 @@ void CameraSession::processRequest(Request *request)
> >  	     << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> >  	     << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
> >  
> > -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > -		const Stream *stream = it->first;
> > -		FrameBuffer *buffer = it->second;
> > -
> > +	for (const auto &[stream, buffer] : buffers) {
> >  		const FrameMetadata &metadata = buffer->metadata();
> >  
> >  		info << " " << streamNames_[stream]
> > @@ -401,10 +394,10 @@ void CameraSession::processRequest(Request *request)
> >  
> >  	if (printMetadata_) {
> >  		const ControlList &requestMetadata = request->metadata();
> > -		for (const auto &ctrl : requestMetadata) {
> > -			const ControlId *id = controls::controls.at(ctrl.first);
> > +		for (const auto &[key, value] : requestMetadata) {
> > +			const ControlId *id = controls::controls.at(key);
> >  			std::cout << "\t" << id->name() << " = "
> > -				  << ctrl.second.toString() << std::endl;
> > +				  << value.toString() << std::endl;
> >  		}
> >  	}
> >
Nejc Galof Feb. 21, 2022, 1:03 p.m. UTC | #3 
Thanks for noticing the mistake.

Signed-off-by: Nejc Galof <galof.nejc@gmail.com>

Nejc Galof

V V pon., 21. feb. 2022 ob 13:57 je oseba Laurent Pinchart <
laurent.pinchart@ideasonboard.com> napisala:

> Hi Nejc,
>
> On Mon, Feb 21, 2022 at 02:49:46PM +0200, Laurent Pinchart wrote:
> > On Sun, Feb 20, 2022 at 01:52:57AM +0100, Nejc Galof wrote:
> > > Use structured bindings range-based for loops for better readability.
> >
> > It looks nicer indeed !
>
> I forgot to mention that the patch is missing your Signed-off-by line.
> Please see https://libcamera.org/contributing.html#submitting-patches.
> You can use the -s option to git commit to add it. For this patch, you
> can just reply to this e-mail with the SoB line and I'll add it locally.
>
> > > ---
> > >  src/cam/camera_session.cpp | 21 +++++++--------------
> > >  1 file changed, 7 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > index 1bf460fa..0428b538 100644
> > > --- a/src/cam/camera_session.cpp
> > > +++ b/src/cam/camera_session.cpp
> > > @@ -120,10 +120,7 @@ CameraSession::~CameraSession()
> > >
> > >  void CameraSession::listControls() const
> > >  {
> > > -   for (const auto &ctrl : camera_->controls()) {
> > > -           const ControlId *id = ctrl.first;
> > > -           const ControlInfo &info = ctrl.second;
> > > -
> > > +   for (const auto &[id, info] : camera_->controls()) {
> > >             std::cout << "Control: " << id->name() << ": "
> > >                       << info.toString() << std::endl;
> > >     }
> >
> > You can now drop the curly braces.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > There's no need to submit a v2, I'll make this small change when
> > applying the patch.
> >
> > > @@ -131,9 +128,8 @@ void CameraSession::listControls() const
> > >
> > >  void CameraSession::listProperties() const
> > >  {
> > > -   for (const auto &prop : camera_->properties()) {
> > > -           const ControlId *id = properties::properties.at
> (prop.first);
> > > -           const ControlValue &value = prop.second;
> > > +   for (const auto &[key, value] : camera_->properties()) {
> > > +           const ControlId *id = properties::properties.at(key);
> > >
> > >             std::cout << "Property: " << id->name() << " = "
> > >                       << value.toString() << std::endl;
> > > @@ -374,10 +370,7 @@ void CameraSession::processRequest(Request
> *request)
> > >          << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> > >          << " (" << std::fixed << std::setprecision(2) << fps << "
> fps)";
> > >
> > > -   for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > > -           const Stream *stream = it->first;
> > > -           FrameBuffer *buffer = it->second;
> > > -
> > > +   for (const auto &[stream, buffer] : buffers) {
> > >             const FrameMetadata &metadata = buffer->metadata();
> > >
> > >             info << " " << streamNames_[stream]
> > > @@ -401,10 +394,10 @@ void CameraSession::processRequest(Request
> *request)
> > >
> > >     if (printMetadata_) {
> > >             const ControlList &requestMetadata = request->metadata();
> > > -           for (const auto &ctrl : requestMetadata) {
> > > -                   const ControlId *id = controls::controls.at
> (ctrl.first);
> > > +           for (const auto &[key, value] : requestMetadata) {
> > > +                   const ControlId *id = controls::controls.at(key);
> > >                     std::cout << "\t" << id->name() << " = "
> > > -                             << ctrl.second.toString() << std::endl;
> > > +                             << value.toString() << std::endl;
> > >             }
> > >     }
> > >
>
> --
> Regards,
>
> Laurent Pinchart
>
Kieran Bingham Feb. 21, 2022, 1:10 p.m. UTC | #4 
Quoting Nejc Galof (2022-02-20 00:52:57)
> Use structured bindings range-based for loops for better readability.
> ---
>  src/cam/camera_session.cpp | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index 1bf460fa..0428b538 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -120,10 +120,7 @@ CameraSession::~CameraSession()
>  
>  void CameraSession::listControls() const
>  {
> -       for (const auto &ctrl : camera_->controls()) {
> -               const ControlId *id = ctrl.first;
> -               const ControlInfo &info = ctrl.second;
> -
> +       for (const auto &[id, info] : camera_->controls()) {

I have continually disliked '.first' and '.second' so I'm happy to see
this.

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


I still prefer the structured bindings, but where we explicitly cast
the type to .first and .second does help show the type information, but
that can also be determined/conveyed from the return type of the
iterator/getters - so I still think this is fine.


>                 std::cout << "Control: " << id->name() << ": "
>                           << info.toString() << std::endl;
>         }
> @@ -131,9 +128,8 @@ void CameraSession::listControls() const
>  
>  void CameraSession::listProperties() const
>  {
> -       for (const auto &prop : camera_->properties()) {
> -               const ControlId *id = properties::properties.at(prop.first);
> -               const ControlValue &value = prop.second;
> +       for (const auto &[key, value] : camera_->properties()) {
> +               const ControlId *id = properties::properties.at(key);
>  
>                 std::cout << "Property: " << id->name() << " = "
>                           << value.toString() << std::endl;
> @@ -374,10 +370,7 @@ void CameraSession::processRequest(Request *request)
>              << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
>              << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
>  
> -       for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> -               const Stream *stream = it->first;
> -               FrameBuffer *buffer = it->second;
> -
> +       for (const auto &[stream, buffer] : buffers) {
>                 const FrameMetadata &metadata = buffer->metadata();
>  
>                 info << " " << streamNames_[stream]
> @@ -401,10 +394,10 @@ void CameraSession::processRequest(Request *request)
>  
>         if (printMetadata_) {
>                 const ControlList &requestMetadata = request->metadata();
> -               for (const auto &ctrl : requestMetadata) {
> -                       const ControlId *id = controls::controls.at(ctrl.first);
> +               for (const auto &[key, value] : requestMetadata) {
> +                       const ControlId *id = controls::controls.at(key);
>                         std::cout << "\t" << id->name() << " = "
> -                                 << ctrl.second.toString() << std::endl;
> +                                 << value.toString() << std::endl;
>                 }
>         }
>  
> -- 
> 2.17.1
>

Patch
diff mbox series 

diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
index 1bf460fa..0428b538 100644
--- a/src/cam/camera_session.cpp
+++ b/src/cam/camera_session.cpp
@@ -120,10 +120,7 @@  CameraSession::~CameraSession()
 
 void CameraSession::listControls() const
 {
-	for (const auto &ctrl : camera_->controls()) {
-		const ControlId *id = ctrl.first;
-		const ControlInfo &info = ctrl.second;
-
+	for (const auto &[id, info] : camera_->controls()) {
 		std::cout << "Control: " << id->name() << ": "
 			  << info.toString() << std::endl;
 	}
@@ -131,9 +128,8 @@  void CameraSession::listControls() const
 
 void CameraSession::listProperties() const
 {
-	for (const auto &prop : camera_->properties()) {
-		const ControlId *id = properties::properties.at(prop.first);
-		const ControlValue &value = prop.second;
+	for (const auto &[key, value] : camera_->properties()) {
+		const ControlId *id = properties::properties.at(key);
 
 		std::cout << "Property: " << id->name() << " = "
 			  << value.toString() << std::endl;
@@ -374,10 +370,7 @@  void CameraSession::processRequest(Request *request)
 	     << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
 	     << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
 
-	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
-		const Stream *stream = it->first;
-		FrameBuffer *buffer = it->second;
-
+	for (const auto &[stream, buffer] : buffers) {
 		const FrameMetadata &metadata = buffer->metadata();
 
 		info << " " << streamNames_[stream]
@@ -401,10 +394,10 @@  void CameraSession::processRequest(Request *request)
 
 	if (printMetadata_) {
 		const ControlList &requestMetadata = request->metadata();
-		for (const auto &ctrl : requestMetadata) {
-			const ControlId *id = controls::controls.at(ctrl.first);
+		for (const auto &[key, value] : requestMetadata) {
+			const ControlId *id = controls::controls.at(key);
 			std::cout << "\t" << id->name() << " = "
-				  << ctrl.second.toString() << std::endl;
+				  << value.toString() << std::endl;
 		}
 	}