[libcamera-devel] test: v4l2_subdevice: list_formats: Port to use utils::hex() output helper

3986 diff mbox series

The hex stream output helper was introduced in f391048a.
It simplifies writing hexadecimal values to an ostream which can be used
here in this test too. As the helper doesn't modify the stream configuration
(refer to utils::hex() documentation), this eliminates the need of restoring
the stream's format state as pointed out by the coverity scan.

Reported-by: Coverity CID=279058
Signed-off-by: Umang Jain <email@uajain.com>
---
 test/v4l2_subdevice/list_formats.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Hi Umang,

On 08/06/2020 14:24, Umang Jain wrote:
> The hex stream output helper was introduced in f391048a.
> It simplifies writing hexadecimal values to an ostream which can be used
> here in this test too. As the helper doesn't modify the stream configuration
> (refer to utils::hex() documentation), this eliminates the need of restoring
> the stream's format state as pointed out by the coverity scan.
> 

Thanks, on it's own this looks good to me, but I wonder if it highlights
that this test has become out-dated.

The test is about v4l2_subdevice formats, and I'm quite sure there is a
V4L2Subdeviceformat (or such) with a .toString() which might be more
appropriate in this test somewhere.

Still, that could always be developed on top too, and this does fix a
coverity issue, and at least make this code tidier.

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


> Reported-by: Coverity CID=279058
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  test/v4l2_subdevice/list_formats.cpp | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> index 25503c3..a55af11 100644
> --- a/test/v4l2_subdevice/list_formats.cpp
> +++ b/test/v4l2_subdevice/list_formats.cpp
> @@ -5,12 +5,12 @@
>   * libcamera V4L2 Subdevice format handling test
>   */
>  
> -#include <iomanip>
>  #include <iostream>
>  #include <vector>
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  
>  #include "v4l2_subdevice_test.h"
> @@ -36,8 +36,7 @@ void ListFormatsTest::printFormats(unsigned int pad,
>  {
>  	cout << "Enumerate formats on pad " << pad << endl;
>  	for (const SizeRange &size : sizes) {
> -		cout << "	mbus code: 0x" << setfill('0') << setw(4)
> -		     << hex << code << endl;
> +		cout << "	mbus code: " << utils::hex(code, 4) << endl;
>  		cout << "	min width: " << dec << size.min.width << endl;
>  		cout << "	min height: " << dec << size.min.height << endl;
>  		cout << "	max width: " << dec << size.max.width << endl;
>

Hello,

On Mon, Jun 08, 2020 at 03:11:59PM +0100, Kieran Bingham wrote:
> On 08/06/2020 14:24, Umang Jain wrote:
> > The hex stream output helper was introduced in f391048a.

Could you please use the format of the Fixes: tags when mentioning
commits (the 12 first characters of the SHA-1, and the subject line) ?
This would be

... was introduced in commit f391048a7b98 ("libcamera: utils: Add hex
stream output helper").

> > It simplifies writing hexadecimal values to an ostream which can be used
> > here in this test too. As the helper doesn't modify the stream configuration
> > (refer to utils::hex() documentation), this eliminates the need of restoring
> > the stream's format state as pointed out by the coverity scan.
> 
> Thanks, on it's own this looks good to me, but I wonder if it highlights
> that this test has become out-dated.
> 
> The test is about v4l2_subdevice formats, and I'm quite sure there is a
> V4L2Subdeviceformat (or such) with a .toString() which might be more
> appropriate in this test somewhere.
> 
> Still, that could always be developed on top too, and this does fix a
> coverity issue, and at least make this code tidier.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Reported-by: Coverity CID=279058
> > Signed-off-by: Umang Jain <email@uajain.com>

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

> > ---
> >  test/v4l2_subdevice/list_formats.cpp | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> > index 25503c3..a55af11 100644
> > --- a/test/v4l2_subdevice/list_formats.cpp
> > +++ b/test/v4l2_subdevice/list_formats.cpp
> > @@ -5,12 +5,12 @@
> >   * libcamera V4L2 Subdevice format handling test
> >   */
> >  
> > -#include <iomanip>
> >  #include <iostream>
> >  #include <vector>
> >  
> >  #include <libcamera/geometry.h>
> >  
> > +#include "libcamera/internal/utils.h"
> >  #include "libcamera/internal/v4l2_subdevice.h"
> >  
> >  #include "v4l2_subdevice_test.h"
> > @@ -36,8 +36,7 @@ void ListFormatsTest::printFormats(unsigned int pad,
> >  {
> >  	cout << "Enumerate formats on pad " << pad << endl;
> >  	for (const SizeRange &size : sizes) {
> > -		cout << "	mbus code: 0x" << setfill('0') << setw(4)
> > -		     << hex << code << endl;
> > +		cout << "	mbus code: " << utils::hex(code, 4) << endl;
> >  		cout << "	min width: " << dec << size.min.width << endl;
> >  		cout << "	min height: " << dec << size.min.height << endl;
> >  		cout << "	max width: " << dec << size.max.width << endl;