[libcamera-devel,RFC,1/6] ipa: raspberrypi: agc: Remove using namespace in agc.hpp
diff mbox series

Message ID 20211005073114.3997303-2-hiroh@chromium.org
State Accepted
Headers show
Series
  • Remove using namespace in header files
Related show

Commit Message

Hirokazu Honda Oct. 5, 2021, 7:31 a.m. UTC 
"using namespace" in a header file propagates the namespace to
the files including the header file. So it should be avoided.
This removes "using namespace" in agc.hpp.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
 src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Oct. 5, 2021, 9:53 a.m. UTC | #1 
Hi Hiro,

(CC'ing David and Naush)

Thank you for the patch.

On Tue, Oct 05, 2021 at 04:31:09PM +0900, Hirokazu Honda wrote:
> "using namespace" in a header file propagates the namespace to
> the files including the header file. So it should be avoided.
> This removes "using namespace" in agc.hpp.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 289c1fce..f6a9cb0a 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -22,6 +22,7 @@
>  using namespace RPiController;
>  using namespace libcamera;
>  using libcamera::utils::Duration;
> +using namespace std::literals::chrono_literals;
>  
>  LOG_DEFINE_CATEGORY(RPiAgc)
>  
> @@ -159,6 +160,12 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
>  	default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
>  }
>  
> +Agc::ExposureValues::ExposureValues()
> +	: shutter(0s), analogue_gain(0),
> +	  total_exposure(0s), total_exposure_no_dg(0s)
> +{
> +}
> +
>  Agc::Agc(Controller *controller)
>  	: AgcAlgorithm(controller), metering_mode_(nullptr),
>  	  exposure_mode_(nullptr), constraint_mode_(nullptr),
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index 82063636..c100d312 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -24,8 +24,6 @@
>  
>  namespace RPiController {
>  
> -using namespace std::literals::chrono_literals;
> -
>  struct AgcMeteringMode {
>  	double weights[AGC_STATS_SIZE];
>  	void Read(boost::property_tree::ptree const &params);
> @@ -112,8 +110,8 @@ private:
>  	uint64_t frame_count_;
>  	AwbStatus awb_;
>  	struct ExposureValues {
> -		ExposureValues() : shutter(0s), analogue_gain(0),
> -				   total_exposure(0s), total_exposure_no_dg(0s) {}
> +		ExposureValues();
> +

I'm a bit more annoyed by this patch than the other ones in the series,
as there's no way to use namespaces explicitly for literals, which leads
to the constructor having to be de-inlined.

As this header is used internally in the Raspberry Pi IPA I don't think
the using directive is a big deal. I don't object to the change though.
I'll let David and Naush decide.

>  		libcamera::utils::Duration shutter;
>  		double analogue_gain;
>  		libcamera::utils::Duration total_exposure;
David Plowman Oct. 5, 2021, 10:20 a.m. UTC | #2 
Hi Hiro, Laurent

Thanks for this change.

On Tue, 5 Oct 2021 at 10:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> (CC'ing David and Naush)
>
> Thank you for the patch.
>
> On Tue, Oct 05, 2021 at 04:31:09PM +0900, Hirokazu Honda wrote:
> > "using namespace" in a header file propagates the namespace to
> > the files including the header file. So it should be avoided.
> > This removes "using namespace" in agc.hpp.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 289c1fce..f6a9cb0a 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -22,6 +22,7 @@
> >  using namespace RPiController;
> >  using namespace libcamera;
> >  using libcamera::utils::Duration;
> > +using namespace std::literals::chrono_literals;
> >
> >  LOG_DEFINE_CATEGORY(RPiAgc)
> >
> > @@ -159,6 +160,12 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> >       default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
> >  }
> >
> > +Agc::ExposureValues::ExposureValues()
> > +     : shutter(0s), analogue_gain(0),
> > +       total_exposure(0s), total_exposure_no_dg(0s)
> > +{
> > +}
> > +
> >  Agc::Agc(Controller *controller)
> >       : AgcAlgorithm(controller), metering_mode_(nullptr),
> >         exposure_mode_(nullptr), constraint_mode_(nullptr),
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 82063636..c100d312 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -24,8 +24,6 @@
> >
> >  namespace RPiController {
> >
> > -using namespace std::literals::chrono_literals;
> > -
> >  struct AgcMeteringMode {
> >       double weights[AGC_STATS_SIZE];
> >       void Read(boost::property_tree::ptree const &params);
> > @@ -112,8 +110,8 @@ private:
> >       uint64_t frame_count_;
> >       AwbStatus awb_;
> >       struct ExposureValues {
> > -             ExposureValues() : shutter(0s), analogue_gain(0),
> > -                                total_exposure(0s), total_exposure_no_dg(0s) {}
> > +             ExposureValues();
> > +
>
> I'm a bit more annoyed by this patch than the other ones in the series,
> as there's no way to use namespaces explicitly for literals, which leads
> to the constructor having to be de-inlined.
>
> As this header is used internally in the Raspberry Pi IPA I don't think
> the using directive is a big deal. I don't object to the change though.
> I'll let David and Naush decide.

I'm fine with this.

Actually, one could argue that we should get rid of agc.hpp and put
the class definition at the top of agc.cpp. Any external access to the
class is meant to be through the AgcAlgorithm base class (and the same
is true of most of our other algorithms). But that's probably one for
another day!

Best regards
David

>
> >               libcamera::utils::Duration shutter;
> >               double analogue_gain;
> >               libcamera::utils::Duration total_exposure;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 5, 2021, 12:07 p.m. UTC | #3 
Hi David,

On Tue, Oct 05, 2021 at 11:20:19AM +0100, David Plowman wrote:
> On Tue, 5 Oct 2021 at 10:53, Laurent Pinchart wrote:
> > On Tue, Oct 05, 2021 at 04:31:09PM +0900, Hirokazu Honda wrote:
> > > "using namespace" in a header file propagates the namespace to
> > > the files including the header file. So it should be avoided.
> > > This removes "using namespace" in agc.hpp.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
> > >  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
> > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > index 289c1fce..f6a9cb0a 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > @@ -22,6 +22,7 @@
> > >  using namespace RPiController;
> > >  using namespace libcamera;
> > >  using libcamera::utils::Duration;
> > > +using namespace std::literals::chrono_literals;
> > >
> > >  LOG_DEFINE_CATEGORY(RPiAgc)
> > >
> > > @@ -159,6 +160,12 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> > >       default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
> > >  }
> > >
> > > +Agc::ExposureValues::ExposureValues()
> > > +     : shutter(0s), analogue_gain(0),
> > > +       total_exposure(0s), total_exposure_no_dg(0s)
> > > +{
> > > +}
> > > +
> > >  Agc::Agc(Controller *controller)
> > >       : AgcAlgorithm(controller), metering_mode_(nullptr),
> > >         exposure_mode_(nullptr), constraint_mode_(nullptr),
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > index 82063636..c100d312 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > @@ -24,8 +24,6 @@
> > >
> > >  namespace RPiController {
> > >
> > > -using namespace std::literals::chrono_literals;
> > > -
> > >  struct AgcMeteringMode {
> > >       double weights[AGC_STATS_SIZE];
> > >       void Read(boost::property_tree::ptree const &params);
> > > @@ -112,8 +110,8 @@ private:
> > >       uint64_t frame_count_;
> > >       AwbStatus awb_;
> > >       struct ExposureValues {
> > > -             ExposureValues() : shutter(0s), analogue_gain(0),
> > > -                                total_exposure(0s), total_exposure_no_dg(0s) {}
> > > +             ExposureValues();
> > > +
> >
> > I'm a bit more annoyed by this patch than the other ones in the series,
> > as there's no way to use namespaces explicitly for literals, which leads
> > to the constructor having to be de-inlined.
> >
> > As this header is used internally in the Raspberry Pi IPA I don't think
> > the using directive is a big deal. I don't object to the change though.
> > I'll let David and Naush decide.
> 
> I'm fine with this.
> 
> Actually, one could argue that we should get rid of agc.hpp and put
> the class definition at the top of agc.cpp. Any external access to the
> class is meant to be through the AgcAlgorithm base class (and the same
> is true of most of our other algorithms). But that's probably one for
> another day!

Can I Have your reviewed-by ?

> > >               libcamera::utils::Duration shutter;
> > >               double analogue_gain;
> > >               libcamera::utils::Duration total_exposure;
David Plowman Oct. 5, 2021, 12:20 p.m. UTC | #4 
Hi Laurent

On Tue, 5 Oct 2021 at 13:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> On Tue, Oct 05, 2021 at 11:20:19AM +0100, David Plowman wrote:
> > On Tue, 5 Oct 2021 at 10:53, Laurent Pinchart wrote:
> > > On Tue, Oct 05, 2021 at 04:31:09PM +0900, Hirokazu Honda wrote:
> > > > "using namespace" in a header file propagates the namespace to
> > > > the files including the header file. So it should be avoided.
> > > > This removes "using namespace" in agc.hpp.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
> > > >  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
> > > >  2 files changed, 9 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > index 289c1fce..f6a9cb0a 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > > > @@ -22,6 +22,7 @@
> > > >  using namespace RPiController;
> > > >  using namespace libcamera;
> > > >  using libcamera::utils::Duration;
> > > > +using namespace std::literals::chrono_literals;
> > > >
> > > >  LOG_DEFINE_CATEGORY(RPiAgc)
> > > >
> > > > @@ -159,6 +160,12 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> > > >       default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
> > > >  }
> > > >
> > > > +Agc::ExposureValues::ExposureValues()
> > > > +     : shutter(0s), analogue_gain(0),
> > > > +       total_exposure(0s), total_exposure_no_dg(0s)
> > > > +{
> > > > +}
> > > > +
> > > >  Agc::Agc(Controller *controller)
> > > >       : AgcAlgorithm(controller), metering_mode_(nullptr),
> > > >         exposure_mode_(nullptr), constraint_mode_(nullptr),
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > index 82063636..c100d312 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > > > @@ -24,8 +24,6 @@
> > > >
> > > >  namespace RPiController {
> > > >
> > > > -using namespace std::literals::chrono_literals;
> > > > -
> > > >  struct AgcMeteringMode {
> > > >       double weights[AGC_STATS_SIZE];
> > > >       void Read(boost::property_tree::ptree const &params);
> > > > @@ -112,8 +110,8 @@ private:
> > > >       uint64_t frame_count_;
> > > >       AwbStatus awb_;
> > > >       struct ExposureValues {
> > > > -             ExposureValues() : shutter(0s), analogue_gain(0),
> > > > -                                total_exposure(0s), total_exposure_no_dg(0s) {}
> > > > +             ExposureValues();
> > > > +
> > >
> > > I'm a bit more annoyed by this patch than the other ones in the series,
> > > as there's no way to use namespaces explicitly for literals, which leads
> > > to the constructor having to be de-inlined.
> > >
> > > As this header is used internally in the Raspberry Pi IPA I don't think
> > > the using directive is a big deal. I don't object to the change though.
> > > I'll let David and Naush decide.
> >
> > I'm fine with this.
> >
> > Actually, one could argue that we should get rid of agc.hpp and put
> > the class definition at the top of agc.cpp. Any external access to the
> > class is meant to be through the AgcAlgorithm base class (and the same
> > is true of most of our other algorithms). But that's probably one for
> > another day!
>
> Can I Have your reviewed-by ?

Oops, sorry!!

Review-by: David Plowman <david.plowman@raspberrypi.com>

David

>
> > > >               libcamera::utils::Duration shutter;
> > > >               double analogue_gain;
> > > >               libcamera::utils::Duration total_exposure;
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
index 289c1fce..f6a9cb0a 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
@@ -22,6 +22,7 @@ 
 using namespace RPiController;
 using namespace libcamera;
 using libcamera::utils::Duration;
+using namespace std::literals::chrono_literals;
 
 LOG_DEFINE_CATEGORY(RPiAgc)
 
@@ -159,6 +160,12 @@  void AgcConfig::Read(boost::property_tree::ptree const &params)
 	default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
 }
 
+Agc::ExposureValues::ExposureValues()
+	: shutter(0s), analogue_gain(0),
+	  total_exposure(0s), total_exposure_no_dg(0s)
+{
+}
+
 Agc::Agc(Controller *controller)
 	: AgcAlgorithm(controller), metering_mode_(nullptr),
 	  exposure_mode_(nullptr), constraint_mode_(nullptr),
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
index 82063636..c100d312 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
@@ -24,8 +24,6 @@ 
 
 namespace RPiController {
 
-using namespace std::literals::chrono_literals;
-
 struct AgcMeteringMode {
 	double weights[AGC_STATS_SIZE];
 	void Read(boost::property_tree::ptree const &params);
@@ -112,8 +110,8 @@  private:
 	uint64_t frame_count_;
 	AwbStatus awb_;
 	struct ExposureValues {
-		ExposureValues() : shutter(0s), analogue_gain(0),
-				   total_exposure(0s), total_exposure_no_dg(0s) {}
+		ExposureValues();
+
 		libcamera::utils::Duration shutter;
 		double analogue_gain;
 		libcamera::utils::Duration total_exposure;