[PATCH v9 2/5] libcamera: geometry: Add two-point Rectangle constructor

[Barnabás Pőcze]

Hi


2024. október 1., kedd 9:50 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:

> Hi Barnabás
> 
> On Mon, Sep 30, 2024 at 10:50:30PM GMT, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2024. szeptember 30., hétfő 21:59 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> >
> > > From: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > >
> > > Add a constructor to the Rectangle class that accepts two points.
> > >
> > > The constructed Rectangle spans all the space between the two given
> > > points.
> > >
> > > 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>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > ---
> > >  include/libcamera/geometry.h |  7 +++++
> > >  src/libcamera/geometry.cpp   | 58 ++++++++++++++++++++++++++++++++++++
> > >  test/geometry.cpp            | 14 +++++++++
> > >  3 files changed, 79 insertions(+)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 3e6f0f5d7fab..2cc25f1facd9 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -262,6 +262,13 @@ public:
> > >  	{
> > >  	}
> > >
> > > +	constexpr Rectangle(const Point &point1, const Point &point2)
> > > +		: Rectangle(std::min(point1.x, point2.x), std::min(point1.y, point2.y),
> > > +			    std::max(point1.x, point2.x) - std::min(point1.x, point2.x),
> > > +			    std::max(point1.y, point2.y) - std::min(point1.y, point2.y))
> >
> > The subtraction can lead to under/overflow. I believe simply casting to
> > `unsigned` avoids that, e.g.:
> >
> >   unsigned(std::max(point1.x, point2.x)) - unsigned(std::min(point1.x, point2.x))
> >
> > Although I don't foresee it causing any issues, since the fix is easy,
> > I think it's worth doing.
> 
> Is this because we're subtracting two signed values and assign the
> result to an unsigned ?

Simply subtracting two signed integers can be problematic. E.g.

  std::numeric_limits<int>::max() - std::numeric_limits<int>::min()

The result is not representable as an `int`. But after casting to `unsigned`,
the subtraction is valid, and result is `std::numeric_limits<unsigned>::max()`
as expected.


Regards,
Barnabás Pőcze


> 
> It shouldn't be an issue, but better safe than sorry, so I'll happily
> take your suggestion in.
> 
> Thanks
>   j
> 
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> > > +	{
> > > +	}
> > > +
> > >  	int x;
> > >  	int y;
> > >  	unsigned int width;
> > > [...]
>