[libcamera-devel] [PATCH 1/3] libcamera: ipa_proxy: Scope ProxyState to IPAProxy

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Apr 13, 2021 at 01:25:32PM +0100, Kieran Bingham wrote:
> The ProxyState is only used by the IPAProxy, so it should remain inside
> that scope.  This helps clarify the usage, and improves the
> documentation by bringing the (future) ProxyState documentation into the
> class.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_proxy.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 9fe446c35f04..ea9f0760c2fc 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -17,15 +17,15 @@ namespace libcamera {
>  
>  class IPAModule;
>  
> -enum ProxyState {
> -	ProxyStopped,
> -	ProxyStopping,
> -	ProxyRunning,
> -};
> -
>  class IPAProxy : public IPAInterface
>  {
>  public:
> +	enum ProxyState {
> +		ProxyStopped,
> +		ProxyStopping,
> +		ProxyRunning,
> +	};

This looks good to me, but I'm wondering if we shouldn't use a scoped
enum instead.

	enum class State {
		Stopped,
		Stopping,
		Running,
	};

Scoped enums require prefixing the enumerator name with the enumeration
name, so code would need to replace ProxyStopped with State::Stopped
(when used inside the IPAProxy class) or IPAPProxy::State::Stopped (when
used outside).

This could be done on top.

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

> +
>  	IPAProxy(IPAModule *ipam);
>  	~IPAProxy();
>  

-- 
Regards,

Laurent Pinchart