[libcamera-devel] [RFC PATCH 1/6] ipa: raspberrypi: agc: Remove using namespace in agc.hpp

[David Plowman]

Hi Laurent

On Tue, 5 Oct 2021 at 13:07, Laurent Pinchart
<laurent.pinchart at 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 at 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 at raspberrypi.com>

David

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