[libcamera-devel,RFC,v1,01/12] libcamera: base: utils: Use size_t for index in utils::enumerate()
diff mbox series

Message ID 20210902042303.2254-2-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Handle fallout of FrameBuffer offset support
Related show

Commit Message

Laurent Pinchart Sept. 2, 2021, 4:22 a.m. UTC 
The index generated by utils::enumerate() is an iteration counter, which
should thus be positive. Use std::size_t instead of the different_type
of the container.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/base/utils.h |  4 ++--
 test/utils.cpp                 | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Kieran Bingham Sept. 2, 2021, 7:48 a.m. UTC | #1 
On 02/09/2021 05:22, Laurent Pinchart wrote:
> The index generated by utils::enumerate() is an iteration counter, which
> should thus be positive. Use std::size_t instead of the different_type
> of the container.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

No other changes required elsewhere?
I guess that means the update is not particularly invasive, and the
existing users will be fine.

Given that looking at the recommended usage for this is with auto, I
guess that means the type is automatically updated for those users:

src/v4l2/v4l2_compat_manager.cpp:
       for (auto [index, camera] : utils::enumerate(cameras)) {

So this looks fine.

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


> ---
>  include/libcamera/base/utils.h |  4 ++--
>  test/utils.cpp                 | 10 +++++-----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index 52301254c2eb..2b761436a99f 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -246,7 +246,7 @@ private:
>  
>  public:
>  	using difference_type = typename std::iterator_traits<Base>::difference_type;
> -	using value_type = std::pair<const difference_type, base_reference>;
> +	using value_type = std::pair<const std::size_t, base_reference>;
>  	using pointer = value_type *;
>  	using reference = value_type &;
>  	using iterator_category = std::input_iterator_tag;
> @@ -275,7 +275,7 @@ public:
>  
>  private:
>  	Base current_;
> -	difference_type pos_;
> +	std::size_t pos_;
>  };
>  
>  template<typename Base>
> diff --git a/test/utils.cpp b/test/utils.cpp
> index d7f810e95e7a..d65467b5102c 100644
> --- a/test/utils.cpp
> +++ b/test/utils.cpp
> @@ -77,8 +77,8 @@ protected:
>  
>  	int testEnumerate()
>  	{
> -		std::vector<int> integers{ 1, 2, 3, 4, 5 };
> -		int i = 0;
> +		std::vector<unsigned int> integers{ 1, 2, 3, 4, 5 };
> +		unsigned int i = 0;
>  
>  		for (auto [index, value] : utils::enumerate(integers)) {
>  			if (index != i || value != i + 1) {
> @@ -93,12 +93,12 @@ protected:
>  			++i;
>  		}
>  
> -		if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) {
> +		if (integers != std::vector<unsigned int>{ 0, 1, 2, 3, 4 }) {
>  			cerr << "Failed to modify container in enumerated range loop" << endl;
>  			return TestFail;
>  		}
>  
> -		Span<const int> span{ integers };
> +		Span<const unsigned int> span{ integers };
>  		i = 0;
>  
>  		for (auto [index, value] : utils::enumerate(span)) {
> @@ -112,7 +112,7 @@ protected:
>  			++i;
>  		}
>  
> -		const int array[] = { 0, 2, 4, 6, 8 };
> +		const unsigned int array[] = { 0, 2, 4, 6, 8 };
>  		i = 0;
>  
>  		for (auto [index, value] : utils::enumerate(array)) {
>
Laurent Pinchart Sept. 2, 2021, 8:28 a.m. UTC | #2 
Hi Kieran,

On Thu, Sep 02, 2021 at 08:48:44AM +0100, Kieran Bingham wrote:
> On 02/09/2021 05:22, Laurent Pinchart wrote:
> > The index generated by utils::enumerate() is an iteration counter, which
> > should thus be positive. Use std::size_t instead of the different_type
> > of the container.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> No other changes required elsewhere?

As far as the compiler tells me :-)

> I guess that means the update is not particularly invasive, and the
> existing users will be fine.
> 
> Given that looking at the recommended usage for this is with auto, I
> guess that means the type is automatically updated for those users:
> 
> src/v4l2/v4l2_compat_manager.cpp:
>        for (auto [index, camera] : utils::enumerate(cameras)) {

Yes. It makes a difference when using index though, is comparing it to a
signed value will now produce a warning (hence the changes in the test).
There are no other usage of the variable that the compiler is unhappy
about.

> So this looks fine.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  include/libcamera/base/utils.h |  4 ++--
> >  test/utils.cpp                 | 10 +++++-----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > index 52301254c2eb..2b761436a99f 100644
> > --- a/include/libcamera/base/utils.h
> > +++ b/include/libcamera/base/utils.h
> > @@ -246,7 +246,7 @@ private:
> >  
> >  public:
> >  	using difference_type = typename std::iterator_traits<Base>::difference_type;
> > -	using value_type = std::pair<const difference_type, base_reference>;
> > +	using value_type = std::pair<const std::size_t, base_reference>;
> >  	using pointer = value_type *;
> >  	using reference = value_type &;
> >  	using iterator_category = std::input_iterator_tag;
> > @@ -275,7 +275,7 @@ public:
> >  
> >  private:
> >  	Base current_;
> > -	difference_type pos_;
> > +	std::size_t pos_;
> >  };
> >  
> >  template<typename Base>
> > diff --git a/test/utils.cpp b/test/utils.cpp
> > index d7f810e95e7a..d65467b5102c 100644
> > --- a/test/utils.cpp
> > +++ b/test/utils.cpp
> > @@ -77,8 +77,8 @@ protected:
> >  
> >  	int testEnumerate()
> >  	{
> > -		std::vector<int> integers{ 1, 2, 3, 4, 5 };
> > -		int i = 0;
> > +		std::vector<unsigned int> integers{ 1, 2, 3, 4, 5 };
> > +		unsigned int i = 0;
> >  
> >  		for (auto [index, value] : utils::enumerate(integers)) {
> >  			if (index != i || value != i + 1) {
> > @@ -93,12 +93,12 @@ protected:
> >  			++i;
> >  		}
> >  
> > -		if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) {
> > +		if (integers != std::vector<unsigned int>{ 0, 1, 2, 3, 4 }) {
> >  			cerr << "Failed to modify container in enumerated range loop" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		Span<const int> span{ integers };
> > +		Span<const unsigned int> span{ integers };
> >  		i = 0;
> >  
> >  		for (auto [index, value] : utils::enumerate(span)) {
> > @@ -112,7 +112,7 @@ protected:
> >  			++i;
> >  		}
> >  
> > -		const int array[] = { 0, 2, 4, 6, 8 };
> > +		const unsigned int array[] = { 0, 2, 4, 6, 8 };
> >  		i = 0;
> >  
> >  		for (auto [index, value] : utils::enumerate(array)) {
> >
Hirokazu Honda Sept. 3, 2021, 9:37 a.m. UTC | #3 
Hi Laurent, thank you for the patch.

On Thu, Sep 2, 2021 at 5:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Thu, Sep 02, 2021 at 08:48:44AM +0100, Kieran Bingham wrote:
> > On 02/09/2021 05:22, Laurent Pinchart wrote:
> > > The index generated by utils::enumerate() is an iteration counter, which
> > > should thus be positive. Use std::size_t instead of the different_type
> > > of the container.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > No other changes required elsewhere?
>
> As far as the compiler tells me :-)
>
> > I guess that means the update is not particularly invasive, and the
> > existing users will be fine.
> >
> > Given that looking at the recommended usage for this is with auto, I
> > guess that means the type is automatically updated for those users:
> >
> > src/v4l2/v4l2_compat_manager.cpp:
> >        for (auto [index, camera] : utils::enumerate(cameras)) {
>
> Yes. It makes a difference when using index though, is comparing it to a
> signed value will now produce a warning (hence the changes in the test).
> There are no other usage of the variable that the compiler is unhappy
> about.
>
> > So this looks fine.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > ---
> > >  include/libcamera/base/utils.h |  4 ++--
> > >  test/utils.cpp                 | 10 +++++-----
> > >  2 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> > > index 52301254c2eb..2b761436a99f 100644
> > > --- a/include/libcamera/base/utils.h
> > > +++ b/include/libcamera/base/utils.h
> > > @@ -246,7 +246,7 @@ private:
> > >
> > >  public:
> > >     using difference_type = typename std::iterator_traits<Base>::difference_type;
> > > -   using value_type = std::pair<const difference_type, base_reference>;
> > > +   using value_type = std::pair<const std::size_t, base_reference>;
> > >     using pointer = value_type *;
> > >     using reference = value_type &;
> > >     using iterator_category = std::input_iterator_tag;
> > > @@ -275,7 +275,7 @@ public:
> > >
> > >  private:
> > >     Base current_;
> > > -   difference_type pos_;
> > > +   std::size_t pos_;
> > >  };
> > >
> > >  template<typename Base>
> > > diff --git a/test/utils.cpp b/test/utils.cpp
> > > index d7f810e95e7a..d65467b5102c 100644
> > > --- a/test/utils.cpp
> > > +++ b/test/utils.cpp
> > > @@ -77,8 +77,8 @@ protected:
> > >
> > >     int testEnumerate()
> > >     {
> > > -           std::vector<int> integers{ 1, 2, 3, 4, 5 };
> > > -           int i = 0;
> > > +           std::vector<unsigned int> integers{ 1, 2, 3, 4, 5 };
> > > +           unsigned int i = 0;
> > >
> > >             for (auto [index, value] : utils::enumerate(integers)) {
> > >                     if (index != i || value != i + 1) {
> > > @@ -93,12 +93,12 @@ protected:
> > >                     ++i;
> > >             }
> > >
> > > -           if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) {
> > > +           if (integers != std::vector<unsigned int>{ 0, 1, 2, 3, 4 }) {
> > >                     cerr << "Failed to modify container in enumerated range loop" << endl;
> > >                     return TestFail;
> > >             }
> > >
> > > -           Span<const int> span{ integers };
> > > +           Span<const unsigned int> span{ integers };
> > >             i = 0;
> > >
> > >             for (auto [index, value] : utils::enumerate(span)) {
> > > @@ -112,7 +112,7 @@ protected:
> > >                     ++i;
> > >             }
> > >
> > > -           const int array[] = { 0, 2, 4, 6, 8 };
> > > +           const unsigned int array[] = { 0, 2, 4, 6, 8 };
> > >             i = 0;
> > >
> > >             for (auto [index, value] : utils::enumerate(array)) {
> > >
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

-Hiro
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
index 52301254c2eb..2b761436a99f 100644
--- a/include/libcamera/base/utils.h
+++ b/include/libcamera/base/utils.h
@@ -246,7 +246,7 @@  private:
 
 public:
 	using difference_type = typename std::iterator_traits<Base>::difference_type;
-	using value_type = std::pair<const difference_type, base_reference>;
+	using value_type = std::pair<const std::size_t, base_reference>;
 	using pointer = value_type *;
 	using reference = value_type &;
 	using iterator_category = std::input_iterator_tag;
@@ -275,7 +275,7 @@  public:
 
 private:
 	Base current_;
-	difference_type pos_;
+	std::size_t pos_;
 };
 
 template<typename Base>
diff --git a/test/utils.cpp b/test/utils.cpp
index d7f810e95e7a..d65467b5102c 100644
--- a/test/utils.cpp
+++ b/test/utils.cpp
@@ -77,8 +77,8 @@  protected:
 
 	int testEnumerate()
 	{
-		std::vector<int> integers{ 1, 2, 3, 4, 5 };
-		int i = 0;
+		std::vector<unsigned int> integers{ 1, 2, 3, 4, 5 };
+		unsigned int i = 0;
 
 		for (auto [index, value] : utils::enumerate(integers)) {
 			if (index != i || value != i + 1) {
@@ -93,12 +93,12 @@  protected:
 			++i;
 		}
 
-		if (integers != std::vector<int>{ 0, 1, 2, 3, 4 }) {
+		if (integers != std::vector<unsigned int>{ 0, 1, 2, 3, 4 }) {
 			cerr << "Failed to modify container in enumerated range loop" << endl;
 			return TestFail;
 		}
 
-		Span<const int> span{ integers };
+		Span<const unsigned int> span{ integers };
 		i = 0;
 
 		for (auto [index, value] : utils::enumerate(span)) {
@@ -112,7 +112,7 @@  protected:
 			++i;
 		}
 
-		const int array[] = { 0, 2, 4, 6, 8 };
+		const unsigned int array[] = { 0, 2, 4, 6, 8 };
 		i = 0;
 
 		for (auto [index, value] : utils::enumerate(array)) {