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

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Reported-by: Coverity CID=279058
> > Signed-off-by: Umang Jain <email at uajain.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at 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;

-- 
Regards,

Laurent Pinchart