[PATCH v4 1/3] libcamera: Add rectangle two-point constructor

[Cheng-Hao Yang]

Hi Barnabás and Jacopo,

On Mon, Sep 16, 2024 at 1:27 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:

> Hi
>
>
> 2024. szeptember 5., csütörtök 10:35 keltezéssel, Jacopo Mondi <
> jacopo.mondi at ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Wed, Sep 04, 2024 at 11:26:37AM GMT, Barnabás Pőcze wrote:
> > > Hi
> > >
> > >
> > > 2024. szeptember 3., kedd 13:39 keltezéssel, Harvey Yang <
> chenghaoyang at chromium.org> írta:
> > >
> > > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > >
> > > > Add a Rectangle constructor that accepts two points:
> > > > topLeft and bottomRight.
> > > >
> > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > ---
> > > >  include/libcamera/geometry.h |  2 ++
> > > >  src/libcamera/geometry.cpp   | 14 ++++++++++++++
> > > >  2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/geometry.h
> b/include/libcamera/geometry.h
> > > > index 3e6f0f5d7..dc56f180f 100644
> > > > --- a/include/libcamera/geometry.h
> > > > +++ b/include/libcamera/geometry.h
> > > > @@ -262,6 +262,8 @@ public:
> > > >   {
> > > >   }
> > > >
> > > > + constexpr Rectangle(const Point &topLeft, const Point
> &bottomRight);
> > >
> > > Don't make this `constexpr` because it is not useful since the
> definition is not available.
> >
> > I found references online that constexpr constuctors are implcitly
> > inline, is this the reason of your comment ?
>
> Yes.
>
>
> >
> > However, I can't find it clearly specified in cppreference. Do you
> > have any pointer ?
>
>   "A constexpr specifier used in a function or static data member(since
> C++17) declaration implies inline."
>   -- https://en.cppreference.com/w/cpp/language/constexpr
>
> >
> > Anyway, if inline is the reason, isn't it better to inline the
> > definition and maintain the constexpr specifier ?
> > [...]
>
> That is an option as well.
>

IIUC, you're suggesting to put the definition of the new c'tor
back in the header file? As we use `ASSERT` in this c'tor
definition, there will be a compile error if we do that:
```

In file included from ../include/libcamera/base/log.h:12,

                 from ../include/libcamera/geometry.h:16,

                 from ../include/libcamera/controls.h:21,

                 from ../include/libcamera/camera.h:22,

                 from ../src/apps/common/dng_writer.h:13,

                 from ../src/apps/common/dng_writer.cpp:8:

../include/libcamera/base/private.h:21:2: error: #error "Private headers
must not be included in the libcamera API"

   21 | #error "Private headers must not be included in the libcamera API"
```

BR,
Harvey


>
> Regards,
> Barnabás Pőcze
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240916/94be57e8/attachment.htm>