[libcamera-devel] [PATCH v7 3/5] lc-compliance: Add Environment singleton

[Kieran Bingham]

Hi Nicolas,

On 15/06/2021 04:33, Laurent Pinchart wrote:
> Hi Nícolas,
> 
> Thank you for the patch.
> 
> On Thu, Jun 10, 2021 at 03:37:30PM -0300, Nícolas F. R. A. Prado wrote:
>> Add a singleton Environment class in order to make the camera available
>> inside all tests. This is needed for the Googletest refactor, otherwise
>> the tests, which are statically declared, won't be able to access the
>> camera.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>> Changes in v7:
>> - Thanks to Jacopo:
>>   - Fixed style issue
>>   - Made CameraManager a unique_ptr and passed to Environment as raw pointer
>>
>> Changes in v6:
>> - Thanks to Niklas:
>>   - Made cameraId() return const& 
>>
>> Changes in v5:
>> - Thanks to Laurent:
>>   - Stored CameraManager and cameraId instead of Camera in Environment
>> - Thanks to Laurent & Niklas:
>>   - Improved Environment singleton class instantiation and destruction
>>
>> Added in v4
>>
>>  src/lc-compliance/environment.cpp | 20 ++++++++++++++++++++
>>  src/lc-compliance/environment.h   | 31 +++++++++++++++++++++++++++++++
>>  src/lc-compliance/meson.build     |  1 +
>>  3 files changed, 52 insertions(+)
>>  create mode 100644 src/lc-compliance/environment.cpp
>>  create mode 100644 src/lc-compliance/environment.h
>>
>> diff --git a/src/lc-compliance/environment.cpp b/src/lc-compliance/environment.cpp
>> new file mode 100644
>> index 000000000000..3a539848362f
>> --- /dev/null
>> +++ b/src/lc-compliance/environment.cpp
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2021, Collabora Ltd.
>> + *
>> + * environment.cpp - Common environment for tests
>> + */
>> +
>> +#include "environment.h"
>> +
>> +Environment *Environment::get()
>> +{
>> +	static Environment instance;
>> +	return &instance;
>> +}
>> +
>> +void Environment::setup(CameraManager* cm, std::string cameraId)
>> +{
>> +	cm_ = cm;
>> +	cameraId_ = cameraId;
>> +}
>> diff --git a/src/lc-compliance/environment.h b/src/lc-compliance/environment.h
>> new file mode 100644
>> index 000000000000..614eb2777800
>> --- /dev/null
>> +++ b/src/lc-compliance/environment.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2021, Collabora Ltd.
>> + *
>> + * environment.h - Common environment for tests
>> + */
>> +#ifndef __LC_COMPLIANCE_ENVIRONMENT_H__
>> +#define __LC_COMPLIANCE_ENVIRONMENT_H__
>> +
>> +#include <libcamera/libcamera.h>
>> +
>> +using namespace libcamera;
>> +
>> +class Environment
>> +{
>> +public:
>> +	static Environment *get();
>> +
>> +	void setup(CameraManager* cm, std::string cameraId);

There are some checkstyle failures in here too. CameraManager *cm
insteaad of CameraManager* cm...

please install checkstyle as a post-commit hook to always get
notifcations about these.

  cp utils/hooks/post-commit .git/hooks


>> +
>> +	const std::string &cameraId() const { return cameraId_; }
>> +	CameraManager* cm() const { return cm_; }
>> +
>> +private:
>> +	Environment() {};
> 
> This breaks compilation with clang:
> 
> ../../src/lc-compliance/environment.h:25:18: error: extra ';' after member function definition [-Werror,-Wextra-semi]
>         Environment() {};
>                         ^
> 1 error generated.
> 
> I'd use
> 
> 	Environment() = default;
> 
> to generate a private default constructor, instead of creating one
> manually.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +
>> +	std::string cameraId_;
>> +	CameraManager* cm_;
>> +};
>> +
>> +#endif /* __LC_COMPLIANCE_ENVIRONMENT_H__ */
>> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
>> index a2bfcceb1259..6dd49085569f 100644
>> --- a/src/lc-compliance/meson.build
>> +++ b/src/lc-compliance/meson.build
>> @@ -12,6 +12,7 @@ lc_compliance_enabled = true
>>  lc_compliance_sources = files([
>>      '../cam/event_loop.cpp',
>>      '../cam/options.cpp',
>> +    'environment.cpp',
>>      'main.cpp',
>>      'results.cpp',
>>      'simple_capture.cpp',
> 

-- 
Regards
--
Kieran