[libcamera-devel] [PATCH] libcamera: utils: Add arithmetic operators for struct timespec

[Laurent Pinchart]

Hi Shik,

On Wed, Jan 23, 2019 at 11:00:21PM +0800, Shik Chen wrote:
> On Wed, Jan 23, 2019 at 4:28 PM Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
> >
> > Implement additions and subtraction of struct timespec through
> > non-member operators to simplify timespec handling.
> 
> IIUC, operator overloading on types of other libs is banned by the style guide
> :Q
> https://google.github.io/styleguide/cppguide.html#Operator_Overloading

But it also states "Don't go out of your way to avoid defining operator
overloads. For example, prefer to define ==, =, and <<, rather than
Equals(), CopyFrom(), and PrintTo().". Would you recommend create
timespecAdd() and timespecSub() functions instead ? I think this is a
good case for operator overloading, as adding or subtracting timespec
structures will be rejected if utils.h isn't included (so no risk of
different behaviours unknown to the developer), and the + and -
operators map very well to what we need to do.

For reference, my use case was

	struct timespec start;
	clock_gettime(CLOCK_MONOTINIC, &start);

	...

	struct timespec now;
	clock_gettime(CLOCK_MONOTINIC, &now);
	struct timespec timeout = old_timeout + start - now;

which could be written

	struct timespec timeout = old_timeout;
	timespecAdd(&timeout, start);
	timespecSub(&timeout, now);

but is a bit less convenient.

What do you advise ?

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >
> > I wrote this code while working on EINTR handling for the event
> > dispatcher, and later refactored the implementation in a way that
> > doesn't make use of these operators anymore. I thought they could still
> > be useful as reference for later use. This patch is thus not meant to be
> > merged now, but can be picked up later if anyone needs it.
> >
> >  src/libcamera/include/utils.h |  6 +++++
> >  src/libcamera/meson.build     |  1 +
> >  src/libcamera/utils.cpp       | 46 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> >  create mode 100644 src/libcamera/utils.cpp
> >
> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> > index 73fa2e69b029..7df20976f36f 100644
> > --- a/src/libcamera/include/utils.h
> > +++ b/src/libcamera/include/utils.h
> > @@ -8,6 +8,7 @@
> >  #define __LIBCAMERA_UTILS_H__
> >
> >  #include <memory>
> > +#include <time.h>
> >
> >  #define ARRAY_SIZE(a)  (sizeof(a) / sizeof(a[0]))
> >
> > @@ -24,6 +25,11 @@ std::unique_ptr<T> make_unique(Args&&... args)
> >
> >  } /* namespace utils */
> >
> > +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> rhs);
> > +struct timespec operator+(struct timespec lhs, const struct timespec &rhs);
> > +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> rhs);
> > +struct timespec operator-(struct timespec lhs, const struct timespec &rhs);
> > +
> >  } /* namespace libcamera */
> >
> >  #endif /* __LIBCAMERA_UTILS_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f9f25c0ecf15..72d4a194e19a 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -11,6 +11,7 @@ libcamera_sources = files([
> >      'pipeline_handler.cpp',
> >      'signal.cpp',
> >      'timer.cpp',
> > +    'utils.cpp',
> >      'v4l2_device.cpp',
> >  ])
> >
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > new file mode 100644
> > index 000000000000..5014b27d8c7d
> > --- /dev/null
> > +++ b/src/libcamera/utils.cpp
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * utils.cpp - Miscellaneous utility functions
> > + */
> > +
> > +#include "utils.h"
> > +
> > +namespace libcamera {
> > +
> > +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> rhs)
> > +{
> > +       lhs.tv_sec += rhs.tv_sec;
> > +       lhs.tv_nsec += rhs.tv_nsec;
> > +       if (lhs.tv_nsec >= 1000000000) {
> > +               lhs.tv_sec++;
> > +               lhs.tv_nsec -= 1000000000;
> > +       }
> > +
> > +       return lhs;
> > +}
> > +
> > +struct timespec operator+(struct timespec lhs, const struct timespec &rhs)
> > +{
> > +       return lhs += rhs;
> > +}
> > +
> > +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> rhs)
> > +{
> > +       lhs.tv_sec -= rhs.tv_sec;
> > +       lhs.tv_nsec -= rhs.tv_nsec;
> > +       if (lhs.tv_nsec < 0) {
> > +               lhs.tv_sec--;
> > +               lhs.tv_nsec += 1000000000;
> > +       }
> > +
> > +       return lhs;
> > +}
> > +
> > +struct timespec operator-(struct timespec lhs, const struct timespec &rhs)
> > +{
> > +       return lhs - rhs;
> > +}
> > +
> > +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart