| Message ID | 20210413122534.342138-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > + > IPAProxy(IPAModule *ipam); > ~IPAProxy(); >
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, + }; + IPAProxy(IPAModule *ipam); ~IPAProxy();
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@ideasonboard.com> --- include/libcamera/internal/ipa_proxy.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)