[libcamera-devel] [PATCH v5 5/5] lc-compliance: Add list and filter parameters

[Nícolas F. R. A. Prado]

Hi Niklas,

On Mon, Jun 07, 2021 at 06:50:50PM +0200, Niklas Söderlund wrote:
> Hi Nícolas,
> 
> On 2021-06-07 12:30:11 -0300, Nícolas F. R. A. Prado wrote:
> > Hi Niklas,
> > 
> > On Fri, Jun 04, 2021 at 11:37:06PM +0200, Niklas Söderlund wrote:
> > > Hi Nícolas,
> > > 
> > > Thanks for your work.
> > > 
> > > On 2021-05-24 16:13:09 -0300, Nícolas F. R. A. Prado wrote:
> > > > Add a --list parameter that lists all current tests (by mapping to
> > > > googletest's --gtest_list_tests).
> > > > 
> > > > Add a --filter 'filterString' parameter that filters the tests to run
> > > > (by mapping to googletest's --gtest_filter).
> > > > 
> > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > > ---
> > > > Changes in v5:
> > > > - Thanks to Niklas:
> > > >   - Moved buildGtestParameters() inside run()
> > > > 
> > > > No changes in v4
> > > > 
> > > >  src/lc-compliance/main.cpp | 71 +++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 66 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > > index 27503372d0eb..182807a14c31 100644
> > > > --- a/src/lc-compliance/main.cpp
> > > > +++ b/src/lc-compliance/main.cpp
> > > > @@ -20,6 +20,8 @@ using namespace libcamera;
> > > >  
> > > >  enum {
> > > >  	OptCamera = 'c',
> > > > +	OptList = 'l',
> > > > +	OptFilter = 'f',
> > > >  	OptHelp = 'h',
> > > >  };
> > > >  
> > > > @@ -45,17 +47,25 @@ public:
> > > >  	~Harness();
> > > >  
> > > >  	int init();
> > > > -	int run(int argc, char **argv);
> > > > +	int run(char *arg0);
> > > > +	int buildGtestParameters(char *arg0);
> > > 
> > > buildGtestParameters() don't need to be public does it?
> > 
> > Right, it doesn't.
> > 
> > > 
> > > >  
> > > >  private:
> > > >  	void listCameras();
> > > >  
> > > >  	OptionsParser::Options options_;
> > > >  	std::shared_ptr<CameraManager> cm_;
> > > > +
> > > > +	const std::map<std::string, std::string> gtestFlag_ = { { "list", "--gtest_list_tests" },
> > > > +								{ "filter", "--gtest_filter" } };
> > > > +
> > > > +	int gtestArgc_;
> > > > +	char **gtestArgv_;
> > > > +	std::string gtestFilterParam_;
> > > >  };
> > > >  
> > > >  Harness::Harness(const OptionsParser::Options &options)
> > > > -	: options_(options)
> > > > +	: options_(options), gtestArgv_(nullptr)
> > > >  {
> > > >  	cm_ = std::make_shared<CameraManager>();
> > > >  }
> > > > @@ -63,6 +73,8 @@ Harness::Harness(const OptionsParser::Options &options)
> > > >  Harness::~Harness()
> > > >  {
> > > >  	cm_->stop();
> > > > +
> > > > +	free(gtestArgv_);
> > > >  }
> > > >  
> > > >  int Harness::init()
> > > > @@ -76,6 +88,9 @@ int Harness::init()
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > +	if (options_.isSet(OptList))
> > > > +		return 0;
> > > > +
> > > >  	if (!options_.isSet(OptCamera)) {
> > > >  		std::cout << "No camera specified, available cameras:" << std::endl;
> > > >  		listCameras();
> > > > @@ -97,9 +112,13 @@ int Harness::init()
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -int Harness::run(int argc, char **argv)
> > > > +int Harness::run(char *arg0)
> > > >  {
> > > > -	::testing::InitGoogleTest(&argc, argv);
> > > > +	int ret = buildGtestParameters(arg0);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	::testing::InitGoogleTest(&gtestArgc_, gtestArgv_);
> > > 
> > > It looks like this could be reworked so gtestArgc_ and gtestArgv_ don't 
> > > need to be class members right. Could not ::testing::InitGoogleTest() be 
> > > called inside buildGtestParameters() (with a possible rename)?
> > 
> > I think we could do this to remove gtestArgc_, but we still need to keep track
> > of gtestArgv_ as a class member so we can free() that pointer in ~Harness(),
> > otherwise we'll leak its memory, right?
> 
> Could we not free it right after calling ::testing::InitGoogleTest() 
> instead of the destructor?

Just tested it and indeed we can. I was under the impression Gtest relied on
that data after the call, but I guess it must copy it internally before
returning then.

I'll change it for v6 as well then.

Thanks,
Nícolas